{"thread":{"id":"56411","subject":"[PATCH 0/7] Add a new --remerge-diff capability to show & log","startedAt":"2021-08-31T02:26:46Z","lastAt":"2021-10-01T16:36:51Z","messageCount":64,"participants":["Elijah Newren via GitGitGadget","Sergey Organov","Bagas Sanjaya","Elijah Newren","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King","Neeraj Singh"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"434217","messageId":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","threadId":"56411","inReplyTo":null,"subject":"[PATCH 0/7] Add a new --remerge-diff capability to show & log","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T02:26:33Z","receivedAt":"2021-08-31T02:26:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Here are some patches to add a --remerge-diff capability to show & log,\nwhich works by comparing merge commits to an automatic remerge (note that\nthe automatic remerge tree can contain files with conflict markers).\n\nHere are some example commits you can try this out on (with git show\n--remerge-diff $COMMIT):\n\n * git.git conflicted merge: 07601b5b36\n * git.git non-conflicted change: bf04590ecd\n * linux.git conflicted merge: eab3540562fb\n * linux.git non-conflicted change: 223cea6a4f05\n\nMany more can be found by just running git log --merges --remerge-diff in\nyour repository of choice and searching for diffs (most merges tend to be\nclean and unmodified and thus produce no diff but a search of '^diff' in the\nlog output tends to find the examples nicely).\n\nSome basic high level details about this new option:\n\n * This option is most naturally compared to --cc, though the output seems\n to be much more understandable to most users than --cc output.\n * Since merges are often clean and unmodified, this new option results in\n an empty diff for most merges.\n * This new option shows things like the removal of conflict markers, which\n hunks users picked from the various conflicted sides to keep or remove,\n and shows changes made outside of conflict markers (which might reflect\n changes needed to resolve semantic conflicts or cleanups of e.g.\n compilation warnings or other additional changes an integrator felt\n belonged in the merged result).\n * This new option does not (currently) work for octopus merges, since\n merge-ort is specific to two-parent merges[1].\n * This option will not work on a read-only or full filesystem[2].\n * We discussed this capability at Git Merge 2020, and one of the\n suggestions was doing a periodic git gc --auto during the operation (due\n to potential new blobs and trees created during the operation). I found a\n way to avoid that; see [2].\n * This option is faster than you'd probably expect; it handles 33.5 merge\n commits per second in linux.git on my computer; see below.\n\nIn regards to the performance point above, the timing for running the\nfollowing command:\n\ntime git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l\n\n\nin linux.git (with v5.4 checked out, since my copy of linux is very out of\ndate) is as follows:\n\nDIFF_FLAG=--cc: 71m 31.536s\nDIFF_FLAG=--remerge-diff: 31m 3.170s\n\n\nNote that there are 62476 merges in this history. Also, output size is:\n\nDIFF_FLAG=--cc: 2169111 lines\nDIFF_FLAG=--remerge-diff: 2458020 lines\n\n\nSo roughly the same amount of output as --cc, as you'd expect.\n\nAs a side note: git log --remerge-diff, when run in various repositories and\nallowed to run all the way back to the beginning(s) of history, is a nice\nstress test of sorts for merge-ort. Especially when users run it for you on\ntheir repositories they are working on, whether intentionally or via a bug\nin a tool triggering that command to be run unexpectedly. Long story short,\nsuch a bug in an internal tool existed last December and this command was\nrun on an internal repository and found a platform-specific bug in merge-ort\non some really old merge commit from that repo. I fixed that bug (a\nSTABLE_QSORT thing) while upstreaming all the merge-ort patches in the mean\ntime, but it was nice getting extra testing. Having more folks run this on\ntheir repositories might be useful extra testing of the new merge strategy.\n\nAlso, I previously mentioned --remerge-diff-only (a flag to show how\ncherry-picks or reverts differ from an automatic cherry-pick or revert, in\naddition to showing how merges differ from an automatic merge). This series\ndoes not include the patches to introduce that option; I'll submit them\nlater.\n\nTwo other things that might be interesting but are not included and which I\nhaven't investigated:\n\n * some mechanism for passing extra merge options through (e.g.\n -Xignore-space-change)\n * a capability to compare the automatic merge to a second automatic merge\n done with different merge options. (Not sure if this would be of interest\n to end users, but might be interesting while developing new a\n --strategy-option, or maybe checking how changing some default in the\n merge algorithm would affect historical merges in various repositories).\n\n[1] I have nebulous ideas of how an Octopus-centric ORT strategy could be\nwritten -- basically, just repeatedly invoking ort and trying to make sure\nnested conflicts can be differentiated. For now, though, a simple warning is\nprinted that octopus merges are not handled and no diff will be shown. [2]\nNew blobs/trees can be written by the three-way merging step. These are\nwritten to a temporary area (via tmp-objdir.c) under the git object store\nthat is cleaned up at the end of the operation, with the new loose objects\nfrom the remerge being cleaned up after each individual merge.\n\nElijah Newren (7):\n merge-ort: mark a few more conflict messages as omittable\n merge-ort: add ability to record conflict messages in a file\n ll-merge: add API for capturing warnings in a strbuf instead of stderr\n merge-ort: capture and print ll-merge warnings in our preferred\n fashion\n tmp-objdir: new API for creating and removing primary object dirs\n show, log: provide a --remerge-diff capability\n doc/diff-options: explain the new --remerge-diff option\n\n Documentation/diff-options.txt | 8 +++\n builtin/log.c | 23 ++++++++\n diff-merges.c | 12 +++++\n ll-merge.c | 51 +++++++++++++-----\n ll-merge.h | 9 ++++\n log-tree.c | 69 ++++++++++++++++++++++++\n merge-ort.c | 96 +++++++++++++++++++++++++++++++---\n merge-recursive.c | 3 ++\n merge-recursive.h | 1 +\n revision.h | 6 ++-\n t/t6404-recursive-merge.sh | 10 +++-\n t/t6406-merge-attr.sh | 10 +++-\n tmp-objdir.c | 29 ++++++++++\n tmp-objdir.h | 16 ++++++\n 14 files changed, 319 insertions(+), 24 deletions(-)\n\n\nbase-commit: c4203212e360b25a1c69467b5a8437d45a373cac\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1080%2Fnewren%2Fremerge-diff-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1080/newren/remerge-diff-v1\nPull-Request: https://github.com/git/git/pull/1080\n-- \ngitgitgadget\n"},{"id":"434218","messageId":"df6e2774f1a5560a598dd8b46131bc6b0a261d4a.1630376800.git.gitgitgadget@gmail.com","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"[PATCH 1/7] merge-ort: mark a few more conflict messages as omittable","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T02:26:34Z","receivedAt":"2021-08-31T02:26:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\npath_msg() has the ability to mark messages as omittable when recording\nconflict messages in an in-tree file. This conflict message file will\nthen be shown by diffing the merge-created tree to the actual merge\ncommit that is created. While all the messages touched in this commit\nare very useful when trying to create a merge initially, early use with\nthe --remerge-diff feature (the only user of this omittable conflict\nmessage capability), suggests that the particular messages marked in\nthis commit are just noise when trying to see what changes users made to\ncreate a merge commit. Mark them as omittable.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 515dc39b7f6..4dbf0a477af 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -2409,7 +2409,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n \t\t */\n \t\tci->path_conflict = 1;\n \t\tif (pair->status == 'A')\n-\t\t\tpath_msg(opt, new_path, 0,\n+\t\t\tpath_msg(opt, new_path, 1,\n \t\t\t\t _(\"CONFLICT (file location): %s added in %s \"\n \t\t\t\t \"inside a directory that was renamed in %s, \"\n \t\t\t\t \"suggesting it should perhaps be moved to \"\n@@ -2417,7 +2417,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,\n \t\t\t\t old_path, branch_with_new_path,\n \t\t\t\t branch_with_dir_rename, new_path);\n \t\telse\n-\t\t\tpath_msg(opt, new_path, 0,\n+\t\t\tpath_msg(opt, new_path, 1,\n \t\t\t\t _(\"CONFLICT (file location): %s renamed to %s \"\n \t\t\t\t \"in %s, inside a directory that was renamed \"\n \t\t\t\t \"in %s, suggesting it should perhaps be \"\n@@ -3814,7 +3814,7 @@ static void process_entry(struct merge_options *opt,\n \t\t\t\treason = _(\"add/add\");\n \t\t\tif (S_ISGITLINK(merged_file.mode))\n \t\t\t\treason = _(\"submodule\");\n-\t\t\tpath_msg(opt, path, 0,\n+\t\t\tpath_msg(opt, path, 1,\n \t\t\t\t _(\"CONFLICT (%s): Merge conflict in %s\"),\n \t\t\t\t reason, path);\n \t\t}\n-- \ngitgitgadget\n\n"},{"id":"434219","messageId":"ed71913886e19ccc276b382de707b4bab7834d12.1630376800.git.gitgitgadget@gmail.com","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"[PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T02:26:35Z","receivedAt":"2021-08-31T02:26:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWe want to add an ability for users to run\n git show --remerge-diff $MERGE_COMMIT\nor even\n git log -p --remerge-diff ...\nand have git show the differences between where the merge machinery\nwould stop and what is recorded in merge commits. However, in such\ncases, stdout is not an appropriate location to dump conflict messages.\nWrite those messages to a file in the merge result (not to the working\ntree or index, but just to a blob recorded in the relevant tree object).\n\nThere are several considerations here:\n * We have to pick file(s) where we write these conflict messages too\n * We want to make it as clear as possible that it's not a real file\n but a set of messages about another file\n * We want conflict messages about a file to appear near the file in\n question in a diff, preferably immediately preceding the file in\n question\n * Related to the above, per-file conflict messages are preferred\n over lumping all conflict messages into one big file\n\nTo achive the above:\n * We put the conflict messages for $filename in\n $filename[0:-1] + \" \" + $filename[-1] + \".conflict_msg\"\n or, in words, we insert a space before the final character of\n the filename and then also add \".conflict_msg\" at the end.\n * We start the file with a \"== Conflict notices for $filename ==\"\n banner\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++-\n merge-recursive.c | 3 ++\n merge-recursive.h | 1 +\n 3 files changed, 82 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 4dbf0a477af..a9e69d9cbb0 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -632,7 +632,11 @@ static void path_msg(struct merge_options *opt,\n \t\t const char *fmt, ...)\n {\n \tva_list ap;\n-\tstruct strbuf *sb = strmap_get(&opt->priv->output, path);\n+\tstruct strbuf *sb;\n+\n+\tif (opt->record_conflict_msgs_in_tree && omittable_hint)\n+\t\treturn; /* Do not record mere hints in tree */\n+\tsb = strmap_get(&opt->priv->output, path);\n \tif (!sb) {\n \t\tsb = xmalloc(sizeof(*sb));\n \t\tstrbuf_init(sb, 0);\n@@ -3531,6 +3535,74 @@ static void write_completed_directory(struct merge_options *opt,\n \tinfo->last_directory_len = strlen(info->last_directory);\n }\n \n+static void put_path_msgs_in_file(struct merge_options *opt,\n+\t\t\t\t const char *path,\n+\t\t\t\t struct merged_info *mi,\n+\t\t\t\t struct directory_versions *dir_metadata)\n+{\n+\tstruct strbuf tmp = STRBUF_INIT, new_path_contents = STRBUF_INIT;\n+\tchar *new_path;\n+\tint new_path_basic_len, unique_counter;\n+\tstruct merged_info *new_mi;\n+\tchar final;\n+\tstruct strbuf *sb = strmap_get(&opt->priv->output, path);\n+\n+\tassert(opt->record_conflict_msgs_in_tree);\n+\tif (!sb)\n+\t\treturn;\n+\n+\t/*\n+\t * Determine a pathname for recording conflict messages. We'd like it\n+\t * to sort just before path, but have a name very similar to what path\n+\t * has.\n+\t */\n+\tstrbuf_addstr(&tmp, path);\n+\tfinal = tmp.buf[tmp.len-1];\n+\tstrbuf_setlen(&tmp, tmp.len-1);\n+\tstrbuf_addf(&tmp, \" %c.conflict_msg\", final);\n+\n+\t/*\n+\t * In extremely unlikely event this filename is not unique, modify it\n+\t * with \".\" suffixes until it is.\n+\t */\n+\tnew_path_basic_len = tmp.len;\n+\tunique_counter = 0;\n+\twhile (strmap_contains(&opt->priv->paths, tmp.buf)) {\n+\t\tstrbuf_setlen(&tmp, new_path_basic_len);\n+\t\tstrbuf_addf(&tmp, \".%d\", ++unique_counter);\n+\t}\n+\n+\t/* Now that we have a unique string, move it to our pool */\n+\tnew_path = mem_pool_strdup(&opt->priv->pool, tmp.buf);\n+\tstrbuf_release(&tmp);\n+\n+\t/* Set up contents we want to place in the file. */\n+\tstrbuf_addf(&new_path_contents, \"== Conflict notices for %s ==\\n\",\n+\t\t path);\n+\tstrbuf_addbuf(&new_path_contents, sb);\n+\n+\t/* Set up new_mi */\n+\tnew_mi = mem_pool_calloc(&opt->priv->pool, 1, sizeof(*new_mi));\n+\tnew_mi->result.mode = 0100644;\n+\tnew_mi->is_null = 0;\n+\tnew_mi->clean = 1;\n+\tnew_mi->basename_offset = mi->basename_offset;\n+\tnew_mi->directory_name = mi->directory_name;\n+\tif (write_object_file(new_path_contents.buf, new_path_contents.len,\n+\t\t\t blob_type, &new_mi->result.oid))\n+\t\tdie(_(\"Unable to add %s to database\"), new_path);\n+\n+\t/*\n+\t * Save new_mi in opt->priv->path (so that something will deallocate\n+\t * it later), and record the entry for it.\n+\t */\n+\tstrmap_put(&opt->priv->paths, new_path, new_mi);\n+\trecord_entry_for_tree(dir_metadata, new_path, new_mi);\n+\n+\t/* Cleanup */\n+\tstrbuf_release(&new_path_contents);\n+}\n+\n /* Per entry merge function */\n static void process_entry(struct merge_options *opt,\n \t\t\t const char *path,\n@@ -3991,6 +4063,8 @@ static void process_entries(struct merge_options *opt,\n \t\t\tstruct conflict_info *ci = (struct conflict_info *)mi;\n \t\t\tprocess_entry(opt, path, ci, &dir_metadata);\n \t\t}\n+\t\tif (!opt->priv->call_depth && opt->record_conflict_msgs_in_tree)\n+\t\t\tput_path_msgs_in_file(opt, path, mi, &dir_metadata);\n \t}\n \ttrace2_region_leave(\"merge\", \"processing\", opt->repo);\n \n@@ -4226,6 +4300,9 @@ void merge_switch_to_result(struct merge_options *opt,\n \t\tstruct string_list olist = STRING_LIST_INIT_NODUP;\n \t\tint i;\n \n+\t\tif (opt->record_conflict_msgs_in_tree)\n+\t\t\tBUG(\"Either display conflict messages or record them in tree, not both\");\n+\n \t\ttrace2_region_enter(\"merge\", \"display messages\", opt->repo);\n \n \t\t/* Hack to pre-allocate olist to the desired size */\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 3355d50e8ad..b14fa15be91 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3697,6 +3697,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n \n \tassert(opt->priv == NULL);\n \n+\t/* Not supported; option specific to merge-ort */\n+\tassert(!opt->record_conflict_msgs_in_tree);\n+\n \t/* Sanity check on repo state; index must match head */\n \tif (repo_index_has_changes(opt->repo, head, &sb)) {\n \t\terr(opt, _(\"Your local changes to the following files would be overwritten by merge:\\n %s\"),\ndiff --git a/merge-recursive.h b/merge-recursive.h\nindex 0795a1d3ec1..2e2fab37f46 100644\n--- a/merge-recursive.h\n+++ b/merge-recursive.h\n@@ -46,6 +46,7 @@ struct merge_options {\n \t/* miscellaneous control options */\n \tconst char *subtree_shift;\n \tunsigned renormalize : 1;\n+\tunsigned record_conflict_msgs_in_tree : 1;\n \n \t/* internal fields used by the implementation */\n \tstruct merge_options_internal *priv;\n-- \ngitgitgadget\n\n"},{"id":"434220","messageId":"2bf5efb5169b71ee7c1da006a74aa67794f1399d.1630376800.git.gitgitgadget@gmail.com","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"[PATCH 3/7] ll-merge: add API for capturing warnings in a strbuf instead of stderr","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T02:26:36Z","receivedAt":"2021-08-31T02:26:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhenever ll-merge encounters a binary file, it prints a warning to\nstderr. However, for the --remerge-diff option we want to add, we need\nto capture all conflict messages and show them in a diff instead of\ndumping them straight to stdout or stderr. Add some new API that will\nallow us to capture this output and display it in our preferred method.\n\nSigned-off-by: Elijah Newren \n---\n ll-merge.c | 51 +++++++++++++++++++++++++++++++++++++++------------\n ll-merge.h | 9 +++++++++\n 2 files changed, 48 insertions(+), 12 deletions(-)\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 261657578c7..4d5bdc12464 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -11,11 +11,13 @@\n #include \"run-command.h\"\n #include \"ll-merge.h\"\n #include \"quote.h\"\n+#include \"strbuf.h\"\n \n struct ll_merge_driver;\n \n typedef int (*ll_merge_fn)(const struct ll_merge_driver *,\n \t\t\t mmbuffer_t *result,\n+\t\t\t struct strbuf *warnings,\n \t\t\t const char *path,\n \t\t\t mmfile_t *orig, const char *orig_name,\n \t\t\t mmfile_t *src1, const char *name1,\n@@ -51,6 +53,7 @@ void reset_merge_attributes(void)\n */\n static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t\t\t mmbuffer_t *result,\n+\t\t\t struct strbuf *warnings,\n \t\t\t const char *path,\n \t\t\t mmfile_t *orig, const char *orig_name,\n \t\t\t mmfile_t *src1, const char *name1,\n@@ -59,6 +62,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t\t\t int marker_size)\n {\n \tmmfile_t *stolen;\n+\tconst char *msg = \"Cannot merge binary files: %s (%s vs. %s)\";\n \tassert(opts);\n \n \t/*\n@@ -71,8 +75,11 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \t} else {\n \t\tswitch (opts->variant) {\n \t\tdefault:\n-\t\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n-\t\t\t\tpath, name1, name2);\n+\t\t\tif (warnings) {\n+\t\t\t\tstrbuf_addstr(warnings, \"Warning: \");\n+\t\t\t\tstrbuf_addf(warnings, msg, path, name1, name2);\n+\t\t\t} else\n+\t\t\t\twarning(msg, path, name1, name2);\n \t\t\t/* fallthru */\n \t\tcase XDL_MERGE_FAVOR_OURS:\n \t\t\tstolen = src1;\n@@ -98,6 +105,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n \n static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \t\t\tmmbuffer_t *result,\n+\t\t\tstruct strbuf *warnings,\n \t\t\tconst char *path,\n \t\t\tmmfile_t *orig, const char *orig_name,\n \t\t\tmmfile_t *src1, const char *name1,\n@@ -114,7 +122,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \t buffer_is_binary(orig->ptr, orig->size) ||\n \t buffer_is_binary(src1->ptr, src1->size) ||\n \t buffer_is_binary(src2->ptr, src2->size)) {\n-\t\treturn ll_binary_merge(drv_unused, result,\n+\t\treturn ll_binary_merge(drv_unused, result, warnings,\n \t\t\t\t path,\n \t\t\t\t orig, orig_name,\n \t\t\t\t src1, name1,\n@@ -138,6 +146,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \n static int ll_union_merge(const struct ll_merge_driver *drv_unused,\n \t\t\t mmbuffer_t *result,\n+\t\t\t struct strbuf *warnings,\n \t\t\t const char *path,\n \t\t\t mmfile_t *orig, const char *orig_name,\n \t\t\t mmfile_t *src1, const char *name1,\n@@ -150,7 +159,7 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused,\n \tassert(opts);\n \to = *opts;\n \to.variant = XDL_MERGE_FAVOR_UNION;\n-\treturn ll_xdl_merge(drv_unused, result, path,\n+\treturn ll_xdl_merge(drv_unused, result, warnings, path,\n \t\t\t orig, orig_name, src1, name1, src2, name2,\n \t\t\t &o, marker_size);\n }\n@@ -180,6 +189,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)\n */\n static int ll_ext_merge(const struct ll_merge_driver *fn,\n \t\t\tmmbuffer_t *result,\n+\t\t\tstruct strbuf *warnings,\n \t\t\tconst char *path,\n \t\t\tmmfile_t *orig, const char *orig_name,\n \t\t\tmmfile_t *src1, const char *name1,\n@@ -362,13 +372,14 @@ static void normalize_file(mmfile_t *mm, const char *path, struct index_state *i\n \t}\n }\n \n-int ll_merge(mmbuffer_t *result_buf,\n-\t const char *path,\n-\t mmfile_t *ancestor, const char *ancestor_label,\n-\t mmfile_t *ours, const char *our_label,\n-\t mmfile_t *theirs, const char *their_label,\n-\t struct index_state *istate,\n-\t const struct ll_merge_options *opts)\n+int ll_merge_with_warnings(mmbuffer_t *result_buf,\n+\t\t\t struct strbuf *warnings,\n+\t\t\t const char *path,\n+\t\t\t mmfile_t *ancestor, const char *ancestor_label,\n+\t\t\t mmfile_t *ours, const char *our_label,\n+\t\t\t mmfile_t *theirs, const char *their_label,\n+\t\t\t struct index_state *istate,\n+\t\t\t const struct ll_merge_options *opts)\n {\n \tstruct attr_check *check = load_merge_attributes();\n \tstatic const struct ll_merge_options default_opts;\n@@ -401,11 +412,27 @@ int ll_merge(mmbuffer_t *result_buf,\n \tif (opts->extra_marker_size) {\n \t\tmarker_size += opts->extra_marker_size;\n \t}\n-\treturn driver->fn(driver, result_buf, path, ancestor, ancestor_label,\n+\treturn driver->fn(driver, result_buf, warnings, path,\n+\t\t\t ancestor, ancestor_label,\n \t\t\t ours, our_label, theirs, their_label,\n \t\t\t opts, marker_size);\n }\n \n+int ll_merge(mmbuffer_t *result_buf,\n+\t const char *path,\n+\t mmfile_t *ancestor, const char *ancestor_label,\n+\t mmfile_t *ours, const char *our_label,\n+\t mmfile_t *theirs, const char *their_label,\n+\t struct index_state *istate,\n+\t const struct ll_merge_options *opts)\n+{\n+\treturn ll_merge_with_warnings(result_buf, NULL, path,\n+\t\t\t\t ancestor, ancestor_label,\n+\t\t\t\t ours, our_label,\n+\t\t\t\t theirs, their_label,\n+\t\t\t\t istate, opts);\n+}\n+\n int ll_merge_marker_size(struct index_state *istate, const char *path)\n {\n \tstatic struct attr_check *check;\ndiff --git a/ll-merge.h b/ll-merge.h\nindex aceb1b24132..a5469918ad4 100644\n--- a/ll-merge.h\n+++ b/ll-merge.h\n@@ -96,6 +96,15 @@ int ll_merge(mmbuffer_t *result_buf,\n \t struct index_state *istate,\n \t const struct ll_merge_options *opts);\n \n+int ll_merge_with_warnings(mmbuffer_t *result_buf,\n+\t\t\t struct strbuf *warnings,\n+\t\t\t const char *path,\n+\t\t\t mmfile_t *ancestor, const char *ancestor_label,\n+\t\t\t mmfile_t *ours, const char *our_label,\n+\t\t\t mmfile_t *theirs, const char *their_label,\n+\t\t\t struct index_state *istate,\n+\t\t\t const struct ll_merge_options *opts);\n+\n int ll_merge_marker_size(struct index_state *istate, const char *path);\n void reset_merge_attributes(void);\n \n-- \ngitgitgadget\n\n"},{"id":"434221","messageId":"ce95b82fc492d48fa6022df424f9a303a1c70ad4.1630376800.git.gitgitgadget@gmail.com","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"[PATCH 4/7] merge-ort: capture and print ll-merge warnings in our preferred fashion","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T02:26:37Z","receivedAt":"2021-08-31T02:26:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nInstead of immediately printing ll-merge warnings to stderr, we save\nthem in our output strbuf. Besides allowing us to move these warnings\nto a special file for --remerge-diff, this has two other benefits for\nregular merges done by merge-ort:\n\n * The deferral of messages ensures we can print all messages about\n any given path together (merge-recursive was known to sometimes\n intersperse messages about other paths, particularly when renames\n were involved).\n\n * The deferral of messages means we can avoid printing spurious\n conflict messages when we just end up aborting due to local user\n modifications in the way. (In contrast to merge-recursive.c which\n prematurely checks for local modifications in the way via\n unpack_trees() and gets the check wrong both in terms of false\n positives and false negatives relative to renames, merge-ort does\n not perform the local modifications in the way check until the\n checkout() step after the full merge has been computed.)\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 11 ++++++++---\n t/t6404-recursive-merge.sh | 10 ++++++++--\n t/t6406-merge-attr.sh | 10 ++++++++--\n 3 files changed, 24 insertions(+), 7 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex a9e69d9cbb0..831f8f3e049 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1741,6 +1741,7 @@ static int merge_3way(struct merge_options *opt,\n \tstruct ll_merge_options ll_opts = {0};\n \tchar *base, *name1, *name2;\n \tint merge_status;\n+\tstruct strbuf warnings = STRBUF_INIT;\n \n \tif (!opt->priv->attr_index.initialized)\n \t\tinitialize_attr_index(opt);\n@@ -1781,10 +1782,14 @@ static int merge_3way(struct merge_options *opt,\n \tread_mmblob(&src1, a);\n \tread_mmblob(&src2, b);\n \n-\tmerge_status = ll_merge(result_buf, path, &orig, base,\n-\t\t\t\t&src1, name1, &src2, name2,\n-\t\t\t\t&opt->priv->attr_index, &ll_opts);\n+\tmerge_status = ll_merge_with_warnings(result_buf, &warnings, path,\n+\t\t\t\t\t &orig, base,\n+\t\t\t\t\t &src1, name1, &src2, name2,\n+\t\t\t\t\t &opt->priv->attr_index, &ll_opts);\n+\tif (warnings.len > 0)\n+\t\tpath_msg(opt, path, 0, \"%s\", warnings.buf);\n \n+\tstrbuf_release(&warnings);\n \tfree(base);\n \tfree(name1);\n \tfree(name2);\ndiff --git a/t/t6404-recursive-merge.sh b/t/t6404-recursive-merge.sh\nindex eaf48e941e2..a986eec0420 100755\n--- a/t/t6404-recursive-merge.sh\n+++ b/t/t6404-recursive-merge.sh\n@@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '\n \tprintf \"\\0\\0\" >binary-file &&\n \tgit add binary-file &&\n \tgit commit -m binary2 &&\n-\ttest_must_fail git merge F >merge.out 2>merge.err &&\n-\tgrep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.err\n+\tif test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n+\tthen\n+\t\ttest_must_fail git merge F >merge.out &&\n+\t\tgrep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.out\n+\telse\n+\t\ttest_must_fail git merge F >merge.out 2>merge.err &&\n+\t\tgrep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.err\n+\tfi\n '\n \n test_expect_success 'mark rename/delete as unmerged' '\ndiff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh\nindex 84946458371..75a7994988a 100755\n--- a/t/t6406-merge-attr.sh\n+++ b/t/t6406-merge-attr.sh\n@@ -221,8 +221,14 @@ test_expect_success 'binary files with union attribute' '\n \tprintf \"two\\0\" >bin.txt &&\n \tgit commit -am two &&\n \n-\ttest_must_fail git merge bin-main 2>stderr &&\n-\tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stderr\n+\tif test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n+\tthen\n+\t\ttest_must_fail git merge bin-main >stdout &&\n+\t\tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stdout\n+\telse\n+\t\ttest_must_fail git merge bin-main 2>stderr &&\n+\t\tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stderr\n+\tfi\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"434222","messageId":"67d3b2b09f9ddda616cdd0d1b12ab7afc73670ed.1630376800.git.gitgitgadget@gmail.com","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"[PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T02:26:38Z","receivedAt":"2021-08-31T02:26:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe tmp_objdir API provides the ability to create temporary object\ndirectories, but was designed with the goal of having subprocesses\naccess these object stores, followed by the main process migrating\nobjects from it to the main object store or just deleting it. The\nsubprocesses would view it as their primary datastore and write to it.\n\nFor the --remerge-diff option we want to add to show & log, we want all\nwrites of intermediate merge results (both blobs and trees) to go to\nthis alternate object store; since those writes will be done by the main\nprocess, we need this \"alternate\" object store to actually be the\nprimary object store. When show & log are done, they'll simply remove\nthis temporary object store.\n\nWe also need one more thing -- `git log --remerge-diff` can cause the\ntemporary object store to fill up with loose objects. Rather than\ntrying to gc that are known garbage anyway, we simply want to know the\nlocation of the temporary object store so we can purge the loose objects\nafter each merge.\n\nSigned-off-by: Elijah Newren \n---\n tmp-objdir.c | 29 +++++++++++++++++++++++++++++\n tmp-objdir.h | 16 ++++++++++++++++\n 2 files changed, 45 insertions(+)\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex b8d880e3626..9f75a75d1c0 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)\n \treturn t->env.v;\n }\n \n+const char *tmp_objdir_path(struct tmp_objdir *t)\n+{\n+\treturn t->path.buf;\n+}\n+\n void tmp_objdir_add_as_alternate(const struct tmp_objdir *t)\n {\n \tadd_to_alternates_memory(t->path.buf);\n }\n+\n+void tmp_objdir_make_primary(struct repository *r, const struct tmp_objdir *t)\n+{\n+\tstruct object_directory *od;\n+\tod = xcalloc(1, sizeof(*od));\n+\n+\tod->path = xstrdup(t->path.buf);\n+\tod->next = r->objects->odb;\n+\tr->objects->odb = od;\n+}\n+\n+void tmp_objdir_remove_as_primary(struct repository *r,\n+\t\t\t\t const struct tmp_objdir *t)\n+{\n+\tstruct object_directory *od;\n+\n+\tod = r->objects->odb;\n+\tif (strcmp(t->path.buf, od->path))\n+\t\tBUG(\"expected %s as primary object store; found %s\",\n+\t\t t->path.buf, od->path);\n+\tr->objects->odb = od->next;\n+\tfree(od->path);\n+\tfree(od);\n+}\ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nindex b1e45b4c75d..6067da24e8c 100644\n--- a/tmp-objdir.h\n+++ b/tmp-objdir.h\n@@ -19,6 +19,7 @@\n *\n */\n \n+struct repository;\n struct tmp_objdir;\n \n /*\n@@ -33,6 +34,11 @@ struct tmp_objdir *tmp_objdir_create(void);\n */\n const char **tmp_objdir_env(const struct tmp_objdir *);\n \n+/*\n+ * Return the path used for the temporary object directory.\n+ */\n+const char *tmp_objdir_path(struct tmp_objdir *t);\n+\n /*\n * Finalize a temporary object directory by migrating its objects into the main\n * object database, removing the temporary directory, and freeing any\n@@ -51,4 +57,14 @@ int tmp_objdir_destroy(struct tmp_objdir *);\n */\n void tmp_objdir_add_as_alternate(const struct tmp_objdir *);\n \n+/*\n+ * Add the temporary object directory as the *primary* object store in the\n+ * current process, turning the previous primary object store into an\n+ * alternate.\n+ */\n+void tmp_objdir_make_primary(struct repository *r,\n+\t\t\t const struct tmp_objdir *t);\n+void tmp_objdir_remove_as_primary(struct repository *r,\n+\t\t\t\t const struct tmp_objdir *t);\n+\n #endif /* TMP_OBJDIR_H */\n-- \ngitgitgadget\n\n"},{"id":"434223","messageId":"ac58cb9aeb587a08754e47815d28e2ce91370e66.1630376800.git.gitgitgadget@gmail.com","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"[PATCH 7/7] doc/diff-options: explain the new --remerge-diff option","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T02:26:40Z","receivedAt":"2021-08-31T02:26:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n Documentation/diff-options.txt | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex c89d530d3d1..b05f1c9f1c9 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -64,6 +64,14 @@ ifdef::git-log[]\n \teach of the parents. Separate log entry and diff is generated\n \tfor each parent.\n +\n+--diff-merges=remerge:::\n+--diff-merges=r:::\n+--remerge-diff:::\n+\tWith this option, two-parent merge commits are remerged to\n+\tcreate a temporary tree object -- potentially containing files\n+\twith conflict markers and such. A diff is then shown between\n+\tthat temporary tree and the actual merge commit.\n++\n --diff-merges=combined:::\n --diff-merges=c:::\n -c:::\n-- \ngitgitgadget\n"},{"id":"434224","messageId":"b75e73384fde4f23296bd02eb40455263f805706.1630376800.git.gitgitgadget@gmail.com","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"[PATCH 6/7] show, log: provide a --remerge-diff capability","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-31T02:26:39Z","receivedAt":"2021-08-31T02:26:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen this option is specified, we remerge all (two parent) merge commits\nand diff the actual merge commit to the automatically created version,\nin order to show how users removed conflict markers, resolved the\ndifferent conflict versions, and potentially added new changes outside\nof conflict regions in order to resolve semantic merge problems (or,\npossibly, just to hide other random changes).\n\nThis capability works by creating a temporary object directory and\nmarking it as the primary object store, so that any blobs or trees\ncreated during the automatic merge, can be easily removed afterwards by\njust deleting all objects from the temporary object directory. We can\ndo this after handling each merge commit, in order to avoid the need to\nworry about doing `git gc --auto` runs while running `git log\n--remerge-diff`.\n\nSigned-off-by: Elijah Newren \n---\n builtin/log.c | 23 +++++++++++++++++\n diff-merges.c | 12 +++++++++\n log-tree.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++\n revision.h | 6 ++++-\n 4 files changed, 109 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 3d7717ba5ca..6c7288716e6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -35,6 +35,8 @@\n #include \"repository.h\"\n #include \"commit-reach.h\"\n #include \"range-diff.h\"\n+#include \"dir.h\"\n+#include \"tmp-objdir.h\"\n \n #define MAIL_DEFAULT_WRAP 72\n #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100\n@@ -51,6 +53,7 @@ static int default_encode_email_headers = 1;\n static int decoration_style;\n static int decoration_given;\n static int use_mailmap_config = 1;\n+static struct tmp_objdir *tmp_objdir;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;\n static const char *fmt_pretty;\n@@ -407,6 +410,17 @@ static int cmd_log_walk(struct rev_info *rev)\n \tint saved_nrl = 0;\n \tint saved_dcctc = 0;\n \n+\tif (rev->remerge_diff) {\n+\t\ttmp_objdir = tmp_objdir_create();\n+\t\tif (!tmp_objdir)\n+\t\t\tdie(_(\"unable to create temporary object directory\"));\n+\t\ttmp_objdir_make_primary(the_repository, tmp_objdir);\n+\n+\t\tstrbuf_init(&rev->remerge_objdir_location, 0);\n+\t\tstrbuf_addstr(&rev->remerge_objdir_location,\n+\t\t\t tmp_objdir_path(tmp_objdir));\n+\t}\n+\n \tif (rev->early_output)\n \t\tsetup_early_output();\n \n@@ -449,6 +463,13 @@ static int cmd_log_walk(struct rev_info *rev)\n \trev->diffopt.no_free = 0;\n \tdiff_free(&rev->diffopt);\n \n+\tif (rev->remerge_diff) {\n+\t\tstrbuf_release(&rev->remerge_objdir_location);\n+\t\ttmp_objdir_remove_as_primary(the_repository, tmp_objdir);\n+\t\ttmp_objdir_destroy(tmp_objdir);\n+\t\ttmp_objdir = NULL;\n+\t}\n+\n \tif (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&\n \t rev->diffopt.flags.check_failed) {\n \t\treturn 02;\n@@ -1943,6 +1964,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--name-status does not make sense\"));\n \tif (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)\n \t\tdie(_(\"--check does not make sense\"));\n+\tif (rev.remerge_diff)\n+\t\tdie(_(\"--remerge_diff does not make sense\"));\n \n \tif (!use_patch_format &&\n \t\t(!rev.diffopt.output_format ||\ndiff --git a/diff-merges.c b/diff-merges.c\nindex d897fd8a293..3a24c45b8e5 100644\n--- a/diff-merges.c\n+++ b/diff-merges.c\n@@ -17,6 +17,7 @@ static void suppress(struct rev_info *revs)\n \trevs->combined_all_paths = 0;\n \trevs->merges_imply_patch = 0;\n \trevs->merges_need_diff = 0;\n+\trevs->remerge_diff = 0;\n }\n \n static void set_separate(struct rev_info *revs)\n@@ -45,6 +46,12 @@ static void set_dense_combined(struct rev_info *revs)\n \trevs->dense_combined_merges = 1;\n }\n \n+static void set_remerge_diff(struct rev_info *revs)\n+{\n+\tsuppress(revs);\n+\trevs->remerge_diff = 1;\n+}\n+\n static diff_merges_setup_func_t func_by_opt(const char *optarg)\n {\n \tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n@@ -57,6 +64,8 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n \t\treturn set_combined;\n \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n \t\treturn set_dense_combined;\n+\telse if (!strcmp(optarg, \"r\") || !strcmp(optarg, \"remerge\"))\n+\t\treturn set_remerge_diff;\n \telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"on\"))\n \t\treturn set_to_default;\n \treturn NULL;\n@@ -113,6 +122,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n \t} else if (!strcmp(arg, \"--cc\")) {\n \t\tset_dense_combined(revs);\n \t\trevs->merges_imply_patch = 1;\n+\t} else if (!strcmp(arg, \"--remerge-diff\")) {\n+\t\tset_remerge_diff(revs);\n+\t\trevs->merges_imply_patch = 1;\n \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n \t\tsuppress(revs);\n \t} else if (!strcmp(arg, \"--combined-all-paths\")) {\ndiff --git a/log-tree.c b/log-tree.c\nindex 6dc4412268b..ed69a4da140 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"commit-reach.h\"\n #include \"config.h\"\n #include \"diff.h\"\n #include \"object-store.h\"\n@@ -7,6 +8,7 @@\n #include \"tag.h\"\n #include \"graph.h\"\n #include \"log-tree.h\"\n+#include \"merge-ort.h\"\n #include \"reflog-walk.h\"\n #include \"refs.h\"\n #include \"string-list.h\"\n@@ -16,6 +18,7 @@\n #include \"line-log.h\"\n #include \"help.h\"\n #include \"range-diff.h\"\n+#include \"dir.h\"\n \n static struct decoration name_decoration = { \"object names\" };\n static int decoration_loaded;\n@@ -902,6 +905,60 @@ static int do_diff_combined(struct rev_info *opt, struct commit *commit)\n \treturn !opt->loginfo;\n }\n \n+static int do_remerge_diff(struct rev_info *opt,\n+\t\t\t struct commit_list *parents,\n+\t\t\t struct object_id *oid,\n+\t\t\t struct commit *commit)\n+{\n+\tstruct merge_options o;\n+\tstruct commit_list *bases;\n+\tstruct merge_result res;\n+\tstruct pretty_print_context ctx = {0};\n+\tstruct strbuf commit1 = STRBUF_INIT;\n+\tstruct strbuf commit2 = STRBUF_INIT;\n+\n+\t/* Setup merge options */\n+\tinit_merge_options(&o, the_repository);\n+\tmemset(&res, 0, sizeof(res));\n+\to.show_rename_progress = 0;\n+\n+\tctx.abbrev = DEFAULT_ABBREV;\n+\tformat_commit_message(parents->item, \"%h (%s)\", &commit1, &ctx);\n+\tformat_commit_message(parents->next->item, \"%h (%s)\", &commit2, &ctx);\n+\to.branch1 = commit1.buf;\n+\to.branch2 = commit2.buf;\n+\to.record_conflict_msgs_in_tree = 1;\n+\n+\t/* Parse the relevant commits and get the merge bases */\n+\tparse_commit_or_die(parents->item);\n+\tparse_commit_or_die(parents->next->item);\n+\tbases = get_merge_bases(parents->item, parents->next->item);\n+\n+\t/* Re-merge the parents */\n+\tmerge_incore_recursive(&o,\n+\t\t\t bases, parents->item, parents->next->item,\n+\t\t\t &res);\n+\n+\t/* Show the diff */\n+\tdiff_tree_oid(&res.tree->object.oid, oid, \"\", &opt->diffopt);\n+\tlog_tree_diff_flush(opt);\n+\n+\t/* Cleanup */\n+\tstrbuf_release(&commit1);\n+\tstrbuf_release(&commit2);\n+\tmerge_finalize(&o, &res);\n+\n+\t/* Clean up the temporary object directory */\n+\tif (opt->remerge_objdir_location.buf != NULL &&\n+\t *opt->remerge_objdir_location.buf != '\\0')\n+\t\tremove_dir_recursively(&opt->remerge_objdir_location,\n+\t\t\t\t REMOVE_DIR_KEEP_TOPLEVEL);\n+\telse\n+\t\tBUG(\"unable to remove temporary object directory\");\n+\n+\treturn !opt->loginfo;\n+}\n+\n /*\n * Show the diff of a commit.\n *\n@@ -936,6 +993,18 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n \t}\n \n \tif (is_merge) {\n+\t\tint octopus = (parents->next->next != NULL);\n+\n+\t\tif (opt->remerge_diff) {\n+\t\t\tif (octopus) {\n+\t\t\t\tshow_log(opt);\n+\t\t\t\tfprintf(opt->diffopt.file,\n+\t\t\t\t\t\"diff: warning: Skipping remerge-diff \"\n+\t\t\t\t\t\"for octopus merges.\\n\");\n+\t\t\t\treturn 1;\n+\t\t\t}\n+\t\t\treturn do_remerge_diff(opt, parents, oid, commit);\n+\t\t}\n \t\tif (opt->combine_merges)\n \t\t\treturn do_diff_combined(opt, commit);\n \t\tif (opt->separate_merges) {\ndiff --git a/revision.h b/revision.h\nindex fbb068da9fb..d6562c52252 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -198,7 +198,8 @@ struct rev_info {\n \t\t\tcombine_merges:1,\n \t\t\tcombined_all_paths:1,\n \t\t\tdense_combined_merges:1,\n-\t\t\tfirst_parent_merges:1;\n+\t\t\tfirst_parent_merges:1,\n+\t\t\tremerge_diff:1;\n \n \t/* Format info */\n \tint\t\tshow_notes;\n@@ -320,6 +321,9 @@ struct rev_info {\n \n \t/* misc. flags related to '--no-kept-objects' */\n \tunsigned keep_pack_cache_flags;\n+\n+\t/* Location where temporary objects for remerge-diff are written. */\n+\tstruct strbuf remerge_objdir_location;\n };\n \n int ref_excluded(struct string_list *, const char *path);\n-- \ngitgitgadget\n\n"},{"id":"434252","messageId":"875yvmgee9.fsf@osv.gnss.ru","threadId":"56411","inReplyTo":"b75e73384fde4f23296bd02eb40455263f805706.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/7] show, log: provide a --remerge-diff capability","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-08-31T09:19:26Z","receivedAt":"2021-08-31T09:19:31Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n\n[...]\n\n> diff --git a/diff-merges.c b/diff-merges.c\n> index d897fd8a293..3a24c45b8e5 100644\n> --- a/diff-merges.c\n> +++ b/diff-merges.c\n> @@ -17,6 +17,7 @@ static void suppress(struct rev_info *revs)\n> \trevs->combined_all_paths = 0;\n> \trevs->merges_imply_patch = 0;\n> \trevs->merges_need_diff = 0;\n> +\trevs->remerge_diff = 0;\n> }\n> \n> static void set_separate(struct rev_info *revs)\n> @@ -45,6 +46,12 @@ static void set_dense_combined(struct rev_info *revs)\n> \trevs->dense_combined_merges = 1;\n> }\n> \n> +static void set_remerge_diff(struct rev_info *revs)\n> +{\n> +\tsuppress(revs);\n> +\trevs->remerge_diff = 1;\n> +}\n> +\n> static diff_merges_setup_func_t func_by_opt(const char *optarg)\n> {\n> \tif (!strcmp(optarg, \"off\") || !strcmp(optarg, \"none\"))\n> @@ -57,6 +64,8 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)\n> \t\treturn set_combined;\n> \telse if (!strcmp(optarg, \"cc\") || !strcmp(optarg, \"dense-combined\"))\n> \t\treturn set_dense_combined;\n> +\telse if (!strcmp(optarg, \"r\") || !strcmp(optarg, \"remerge\"))\n> +\t\treturn set_remerge_diff;\n> \telse if (!strcmp(optarg, \"m\") || !strcmp(optarg, \"on\"))\n> \t\treturn set_to_default;\n> \treturn NULL;\n> @@ -113,6 +122,9 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)\n> \t} else if (!strcmp(arg, \"--cc\")) {\n> \t\tset_dense_combined(revs);\n> \t\trevs->merges_imply_patch = 1;\n> +\t} else if (!strcmp(arg, \"--remerge-diff\")) {\n> +\t\tset_remerge_diff(revs);\n> +\t\trevs->merges_imply_patch = 1;\n> \t} else if (!strcmp(arg, \"--no-diff-merges\")) {\n> \t\tsuppress(revs);\n> \t} else if (!strcmp(arg, \"--combined-all-paths\")) {\n\nThe diff-merges options handling looks fine to me.\n\nThanks,\n-- Sergey Organov\n"},{"id":"434260","messageId":"665c48f7-b597-154f-6ac7-8d0471f14869@gmail.com","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] Add a new --remerge-diff capability to show & log","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-08-31T11:05:55Z","receivedAt":"2021-08-31T11:06:01Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 31/08/21 09.26, Elijah Newren via GitGitGadget wrote:\n> Here are some patches to add a --remerge-diff capability to show & log,\n> which works by comparing merge commits to an automatic remerge (note that\n> the automatic remerge tree can contain files with conflict markers).\n> \n> Here are some example commits you can try this out on (with git show\n> --remerge-diff $COMMIT):\n> \n> * git.git conflicted merge: 07601b5b36\n> * git.git non-conflicted change: bf04590ecd\n> * linux.git conflicted merge: eab3540562fb\n> * linux.git non-conflicted change: 223cea6a4f05\n> \n...\n> In regards to the performance point above, the timing for running the\n> following command:\n> \n> time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l\n> \n> \n> in linux.git (with v5.4 checked out, since my copy of linux is very out of\n> date) is as follows:\n> \n> DIFF_FLAG=--cc: 71m 31.536s\n> DIFF_FLAG=--remerge-diff: 31m 3.170s\n> \n> \n> Note that there are 62476 merges in this history. Also, output size is:\n> \n> DIFF_FLAG=--cc: 2169111 lines\n> DIFF_FLAG=--remerge-diff: 2458020 lines\n> \n\nWhich repo did you mean by linux.git? Kernel developers often work \nagainst Linus' mainline tree [1], while end-users (including myself) \nprefer stable tree (which is mainline + stable release branches and \ntags) [2].\n\n[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git\n[2]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"434296","messageId":"CABPp-BGYoa9yfSEv5-XNbUqTrpgKjdNaYsPNSujNuEBfT88=FQ@mail.gmail.com","threadId":"56411","inReplyTo":"665c48f7-b597-154f-6ac7-8d0471f14869@gmail.com","subject":"Re: [PATCH 0/7] Add a new --remerge-diff capability to show & log","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-31T16:16:07Z","receivedAt":"2021-08-31T16:16:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 31, 2021 at 4:05 AM Bagas Sanjaya wrote:\n>\n> On 31/08/21 09.26, Elijah Newren via GitGitGadget wrote:\n> > Here are some patches to add a --remerge-diff capability to show & log,\n> > which works by comparing merge commits to an automatic remerge (note that\n> > the automatic remerge tree can contain files with conflict markers).\n> >\n> > Here are some example commits you can try this out on (with git show\n> > --remerge-diff $COMMIT):\n> >\n> > * git.git conflicted merge: 07601b5b36\n> > * git.git non-conflicted change: bf04590ecd\n> > * linux.git conflicted merge: eab3540562fb\n> > * linux.git non-conflicted change: 223cea6a4f05\n> >\n> ...\n> > In regards to the performance point above, the timing for running the\n> > following command:\n> >\n> > time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l\n> >\n> >\n> > in linux.git (with v5.4 checked out, since my copy of linux is very out of\n> > date) is as follows:\n> >\n> > DIFF_FLAG=--cc: 71m 31.536s\n> > DIFF_FLAG=--remerge-diff: 31m 3.170s\n> >\n> >\n> > Note that there are 62476 merges in this history. Also, output size is:\n> >\n> > DIFF_FLAG=--cc: 2169111 lines\n> > DIFF_FLAG=--remerge-diff: 2458020 lines\n> >\n>\n> Which repo did you mean by linux.git? Kernel developers often work\n> against Linus' mainline tree [1], while end-users (including myself)\n> prefer stable tree (which is mainline + stable release branches and\n> tags) [2].\n>\n> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git\n> [2]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git\n\n$ git remote -v\norigin git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git\n(fetch)\norigin git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git\n(push)\n$ git log -1 origin/master\ncommit 11a48a5a18c63fd7621bb050228cebf13566e4d8 (tag: v5.6-rc2,\norigin/master, origin/HEAD, master)\nAuthor: Linus Torvalds \nDate: Sun Feb 16 13:16:59 2020 -0800\n\n Linux 5.6-rc2\n$ git log -1 HEAD\ncommit 219d54332a09e8d8741c1e1982f5eae56099de85 (HEAD, tag: v5.4)\nAuthor: Linus Torvalds \nDate: Sun Nov 24 16:32:01 2019 -0800\n\n Linux 5.4\n"},{"id":"434329","messageId":"xmqq4kb5tm9f.fsf@gitster.g","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] Add a new --remerge-diff capability to show & log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-31T20:03:24Z","receivedAt":"2021-08-31T20:03:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Here are some patches to add a --remerge-diff capability to show & log,\n> which works by comparing merge commits to an automatic remerge (note that\n> the automatic remerge tree can contain files with conflict markers).\n\nExcited ;-)\n\n> * This new option does not (currently) work for octopus merges, since\n> merge-ort is specific to two-parent merges[1].\n\nUnless you do so manually, the native \"octopus\" backend does not let\nyou create non-trivial merges anyway, so punting on them should not\nbe a big loss. Falling back to --cc might be a usable alternative.\n\n> * This option will not work on a read-only or full filesystem[2].\n\nOK. I am not sure if it is worth doing the \"temporary objects\"\ntrick, though---would it risk repository corruption if somebody is\ncreating a new blob that happens to be identical to the one that is\ninvolved in the remerge operation at the same time, or there is no\nvisibility of the temporary area to these \"somebody\" outside so\nthere is no risk?\n"},{"id":"434332","messageId":"CABPp-BHjpNLXeGA1=KniL=vVuH32c59_LphPZcqnM30GPerCZw@mail.gmail.com","threadId":"56411","inReplyTo":"xmqq4kb5tm9f.fsf@gitster.g","subject":"Re: [PATCH 0/7] Add a new --remerge-diff capability to show & log","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-08-31T20:23:04Z","receivedAt":"2021-08-31T20:23:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 31, 2021 at 1:03 PM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > Here are some patches to add a --remerge-diff capability to show & log,\n> > which works by comparing merge commits to an automatic remerge (note that\n> > the automatic remerge tree can contain files with conflict markers).\n>\n> Excited ;-)\n>\n> > * This new option does not (currently) work for octopus merges, since\n> > merge-ort is specific to two-parent merges[1].\n>\n> Unless you do so manually, the native \"octopus\" backend does not let\n> you create non-trivial merges anyway, so punting on them should not\n> be a big loss. Falling back to --cc might be a usable alternative.\n>\n> > * This option will not work on a read-only or full filesystem[2].\n>\n> OK. I am not sure if it is worth doing the \"temporary objects\"\n> trick, though---would it risk repository corruption if somebody is\n> creating a new blob that happens to be identical to the one that is\n> involved in the remerge operation at the same time, or there is no\n> visibility of the temporary area to these \"somebody\" outside so\n> there is no risk?\n\nThe temporary area is only used by the process running --remerge-diff,\nso there's no risk of corruption. If you have two `git log\n--remerge-diff ...` processes running at the same time, they each have\ntheir own temporary areas.\n"},{"id":"434365","messageId":"xmqqzgsxs4rk.fsf@gitster.g","threadId":"56411","inReplyTo":"df6e2774f1a5560a598dd8b46131bc6b0a261d4a.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/7] merge-ort: mark a few more conflict messages as omittable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-31T21:06:39Z","receivedAt":"2021-08-31T21:06:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> path_msg() has the ability to mark messages as omittable when recording\n> conflict messages in an in-tree file. This conflict message file will\n> then be shown by diffing the merge-created tree to the actual merge\n> commit that is created. While all the messages touched in this commit\n> are very useful when trying to create a merge initially, early use with\n> the --remerge-diff feature (the only user of this omittable conflict\n> message capability), suggests that the particular messages marked in\n> this commit are just noise when trying to see what changes users made to\n> create a merge commit. Mark them as omittable.\n\nSorry for asking something that may be obvious, but if you recreate\nan automated and potentially conflicting merge in-core, in order to\nthen compare the recorded merge result, is there *any* message that\nis worth showing while doing the first step, and where in the output\ndo users see them?\n\n"},{"id":"434372","messageId":"CABPp-BEZjMLGFxSsTfRnAffom9CKRU4if4PsdTh+85L6D=FseA@mail.gmail.com","threadId":"56411","inReplyTo":"xmqqzgsxs4rk.fsf@gitster.g","subject":"Re: [PATCH 1/7] merge-ort: mark a few more conflict messages as omittable","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-01T00:03:07Z","receivedAt":"2021-09-01T00:04:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Aug 31, 2021 at 2:06 PM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > path_msg() has the ability to mark messages as omittable when recording\n> > conflict messages in an in-tree file. This conflict message file will\n> > then be shown by diffing the merge-created tree to the actual merge\n> > commit that is created. While all the messages touched in this commit\n> > are very useful when trying to create a merge initially, early use with\n> > the --remerge-diff feature (the only user of this omittable conflict\n> > message capability), suggests that the particular messages marked in\n> > this commit are just noise when trying to see what changes users made to\n> > create a merge commit. Mark them as omittable.\n>\n> Sorry for asking something that may be obvious\n\nNo, it's not obvious. I had a hard time figuring out the right way to\nsplit up this series and order the patches in part because so many\nlittle pieces are needed before I can show the solution. So some of\nthis comes from the later patches, but let me see if I can motivate\nthe issue and solution I picked a bit more right here.\n\n> but if you recreate\n> an automated and potentially conflicting merge in-core, in order to\n> then compare the recorded merge result, is there *any* message that\n> is worth showing while doing the first step,\n\nYes, absolutely. While three-way *content* conflicts are easily\nrepresentable within a file in the working tree using conflict\nmarkers, *non-content* conflicts are usually not easily representable\nin such a fashion. For example, a rename/delete conflict will not\nresult in any conflict markers, and will result in the renamed version\nof the file being left in the working tree; the only way you know\nthere is a conflict for that file is either because of the stage in\nthe index or the the message that is printed to the terminal:\n\n CONFLICT (rename/delete): %s renamed to %s in %s, but deleted in %s.\n\nBut relying on higher order stages in the index and messages printed\nto the terminal present a bit of a problem for --remerge-diff; as\ndescribed, it ignores both those sources of input. Here's a few other\nconflict types that face similar issues:\n\n * modify/delete conflicts\n * failure to merge submodule\n * directory rename detection (i.e. new file added to old directory\nthat other side renamed; which directory should file end up in?)\n * distinct types of files (e.g. regular file and symlink) located at\nthe same path\n * rename/rename conflicts\n\nIn all these cases (and some others), relying on a diff of \"what the\nworking directory looks like at the end of a merge\" to \"the tree\nrecorded in the merge commit\" does not convey enough (if any)\ninformation about the above types of conflicts.\n\n> and where in the output do users see them?\n\nFor --remerge-diff, I chose to handle the fact that the working\ndirectory didn't naturally have enough information, by augmenting the\nworking directory with additional information. So, for example, if\nthere was a file named `dir/my.file` that had a modify/delete conflict\n(and the user who did the real merge edited that file a bit as part of\nconflict resolution), then I would also add a `dir/my.fil\ne.conflict_msg` file whose contents are\n\n\"\"\"\n== Conflict notices for my.file ==\nCONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT\nSUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2\n(SHORT SUMMARY2) of dir/my.file left in tree.\n\"\"\"\n\nCreating this file means you see something like this in the\nremerge-diff (note there are diffs for two files, with the synthetic\nfile appearing just before the file it has messages about):\n\ndiff --git a/dir/my.fil e.conflict_msg b/dir/my.fil e.conflict_msg\ndeleted file mode 100644\nindex 2bd215a32f06..000000000000\n--- a/dir/my.fil e.conflict_msg\n+++ /dev/null\n@@ -1,2 +0,0 @@\n-== Conflict notices for my.file ==\n-CONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT\nSUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2\n(SHORT SUMMARY2) of dir/my.file left in tree.\ndiff --git a/dir/my.file b/dir/my.file\nindex 09c78c725a3b..79f9d1fb7611 100644\n--- a/dir/my.file\n+++ b/dir/my.file\n@@ -950,15 +912,15 @@ int my_func(struct stuff *data,\n rq->timeline,\n rq);\n\n- old_func(data, 1);\n+ new_func(\"for funsies\", data, VALID);\n obj->value = 8;\n obj->read_attempts = 0;\n } else {\n- err = old_func(data, 0);\n+ err = new_func(\"for funsies\", data, INVALID);\n if (unlikely(err))\n return err;\n\n- another_old_func(data, obj->value);\n+ another_new_func(data, obj->value, INVALID);\n obj->value++;\n }\n obj->read_attempts += 1;\n\n\nThe `dir/my.fil e.conflict_msg` file is definitely slightly weird, but\nany solution here would be. I think it's fairly intuitive what is\nmeant. No one has commented on this choice in the 9+ months it's been\nin use internally by ~50 users (even with -p implying --remerge-diff\nautomatically to make it more likely people have used this option), so\neither it really is intuitive, or it doesn't come up much. It could\nwell be the latter.\n"},{"id":"434458","messageId":"xmqqk0k0qkmv.fsf@gitster.g","threadId":"56411","inReplyTo":"CABPp-BEZjMLGFxSsTfRnAffom9CKRU4if4PsdTh+85L6D=FseA@mail.gmail.com","subject":"Re: [PATCH 1/7] merge-ort: mark a few more conflict messages as omittable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T17:19:04Z","receivedAt":"2021-09-01T17:19:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Creating this file means you see something like this in the\n> remerge-diff (note there are diffs for two files, with the synthetic\n> file appearing just before the file it has messages about):\n>\n> diff --git a/dir/my.fil e.conflict_msg b/dir/my.fil e.conflict_msg\n> deleted file mode 100644\n> index 2bd215a32f06..000000000000\n> --- a/dir/my.fil e.conflict_msg\n> +++ /dev/null\n> @@ -1,2 +0,0 @@\n> -== Conflict notices for my.file ==\n> -CONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT\n> SUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2\n> (SHORT SUMMARY2) of dir/my.file left in tree.\n\nI do not know if my reaction should be \"Cute\" or \"Yuck\" ;-)\n\nHopefully nobody uses .conflict_msg as a suffix for their files.\n"},{"id":"434473","messageId":"xmqqy28gngwj.fsf@gitster.g","threadId":"56411","inReplyTo":"pull.1080.git.git.1630376800.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/7] Add a new --remerge-diff capability to show & log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T21:07:56Z","receivedAt":"2021-09-01T21:08:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Here are some patches to add a --remerge-diff capability to show & log,\n\nOne sad omission from the maintainer usecase is that we do not seem\nto know \"git diff --remerge-diff\" yet during a conflicted merge.\n\n\"git diff [-- ]\" before recording the resolution for the path\nwith \"git add \" shows combined patch to give a final sanity\ncheck before committing it to the rerere database. I am wondering\nif viewing it in the --remrege-diff format instead would help this\nstep even more.\n\n\n\n"},{"id":"434475","messageId":"CABPp-BENXw11v3KxkFL+NsF5Qo7Pff1QdyxG2YxQB+SV6Or-Hg@mail.gmail.com","threadId":"56411","inReplyTo":"xmqqy28gngwj.fsf@gitster.g","subject":"Re: [PATCH 0/7] Add a new --remerge-diff capability to show & log","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-01T21:42:16Z","receivedAt":"2021-09-01T21:42:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 1, 2021 at 2:08 PM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > Here are some patches to add a --remerge-diff capability to show & log,\n>\n> One sad omission from the maintainer usecase is that we do not seem\n> to know \"git diff --remerge-diff\" yet during a conflicted merge.\n>\n> \"git diff [-- ]\" before recording the resolution for the path\n> with \"git add \" shows combined patch to give a final sanity\n> check before committing it to the rerere database. I am wondering\n> if viewing it in the --remrege-diff format instead would help this\n> step even more.\n\nWe do have `git diff AUTO_MERGE`, though. It's not quite the same as\nit doesn't include all the \"CONFLICT\" messages shown in the terminal\nlike --remerge-diff does with log/show, but otherwise it's the same.\nPerhaps we could even alias `git diff --remerge-diff` to `git diff\nAUTO_MERGE`?\n\nSee commit 5291828df838 (merge-ort: write $GIT_DIR/AUTO_MERGE whenever\nwe hit a conflict, 2021-03-20) for more details.\n"},{"id":"434477","messageId":"xmqqtuj4nepe.fsf@gitster.g","threadId":"56411","inReplyTo":"CABPp-BENXw11v3KxkFL+NsF5Qo7Pff1QdyxG2YxQB+SV6Or-Hg@mail.gmail.com","subject":"Re: [PATCH 0/7] Add a new --remerge-diff capability to show & log","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T21:55:25Z","receivedAt":"2021-09-01T21:55:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> We do have `git diff AUTO_MERGE`, though. It's not quite the same as\n> it doesn't include all the \"CONFLICT\" messages shown in the terminal\n> like --remerge-diff does with log/show, but otherwise it's the same.\n\nAh, forgot about that one, so we are good.\n\n> Perhaps we could even alias `git diff --remerge-diff` to `git diff\n> AUTO_MERGE`?\n\nI do not think it is a good idea to hide AUTO_MERGE behind an\noption. It is a feature that deserves more user awareness by\nitself.\n\nThanks.\n"},{"id":"437262","messageId":"87r1d9xh71.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"67d3b2b09f9ddda616cdd0d1b12ab7afc73670ed.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T07:55:11Z","receivedAt":"2021-09-28T08:01:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n\nI commented just now on how this API is duplicated between here &\nanother in-flight series in\nhttps://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/; Just\ncomments on this patch here:\n\n> diff --git a/tmp-objdir.c b/tmp-objdir.c\n> index b8d880e3626..9f75a75d1c0 100644\n> --- a/tmp-objdir.c\n> +++ b/tmp-objdir.c\n> @@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)\n> \treturn t->env.v;\n> }\n> \n> +const char *tmp_objdir_path(struct tmp_objdir *t)\n> +{\n> +\treturn t->path.buf;\n> +}\n\nNot your fault & this pre-dates your patch, but FWIW I prefer our APIs\nthat don't have these \"hidden struct\" shenanigans (like say \"struct\nstring_list\") so a caller could just access this, we can just declare it\n\"const\" appropriately.\n\nWe're also all in-tree friends here, so having various accessors for no\nreason other than to access struct members seems a bit too much.\n\nAll of which is to say if you + Neeraj come up with some common API I\nfor one wouldn't mind moving the struct deceleration to tmp-objdir.h,\nbut it looks like in his version it can stay \"private\" more easily.\n\nIn this particular case I hadn't understood on a previous reading of\ntmp-objdir.c why this \"path\" isn't a statically allocated \"char\npath[PATH_MAX]\", or a least we that hardcoded \"1024\" should be a\nPATH_MAX, as it is in some other cases.\n\nBut I digress :)\n"},{"id":"437263","messageId":"87mtnxxgz3.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"b75e73384fde4f23296bd02eb40455263f805706.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/7] show, log: provide a --remerge-diff capability","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-28T08:01:03Z","receivedAt":"2021-09-28T08:05:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n\n> static int decoration_given;\n> static int use_mailmap_config = 1;\n> +static struct tmp_objdir *tmp_objdir;\n> static const char *fmt_patch_subject_prefix = \"PATCH\";\n> static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;\n> static const char *fmt_pretty;\n\nSo here we make this static file-level etc...\n\n> @@ -407,6 +410,17 @@ static int cmd_log_walk(struct rev_info *rev)\n> \tint saved_nrl = 0;\n> \tint saved_dcctc = 0;\n> \n> +\tif (rev->remerge_diff) {\n> +\t\ttmp_objdir = tmp_objdir_create();\n> +\t\tif (!tmp_objdir)\n> +\t\t\tdie(_(\"unable to create temporary object directory\"));\n> +\t\ttmp_objdir_make_primary(the_repository, tmp_objdir);\n> +\n> +\t\tstrbuf_init(&rev->remerge_objdir_location, 0);\n> +\t\tstrbuf_addstr(&rev->remerge_objdir_location,\n> +\t\t\t tmp_objdir_path(tmp_objdir));\n> +\t}\n> +\n> \tif (rev->early_output)\n> \t\tsetup_early_output();\n> \n> @@ -449,6 +463,13 @@ static int cmd_log_walk(struct rev_info *rev)\n> \trev->diffopt.no_free = 0;\n> \tdiff_free(&rev->diffopt);\n> \n> +\tif (rev->remerge_diff) {\n> +\t\tstrbuf_release(&rev->remerge_objdir_location);\n> +\t\ttmp_objdir_remove_as_primary(the_repository, tmp_objdir);\n> +\t\ttmp_objdir_destroy(tmp_objdir);\n> +\t\ttmp_objdir = NULL;\n\n...but all of the \"tmp_objdir\" usage is in one function, can't the\nvariable be declared here instead?\n\nWe need to hand the \"remerge_objdir_location\" off to the \"rev_info\"\nstruct, but that seems separate from its lifetime.\n\nRe my [1] & [2] I like Neeraj's \"atexit cleanup\" approach better,\nperhaps that makes your cleanup in log-tree.c redundant or easier?\n\nPer [2] it looks like you need to \"hand off\" the\n\"remerge_objdir_location\", so having the struct live in tmp-objdir.h as\nI suggested in [2] might make that work...\n\n1. https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/\n2. https://lore.kernel.org/git/87r1d9xh71.fsf@evledraar.gmail.com/\n"},{"id":"437371","messageId":"YVOXPTjsp9lrxmS6@coredump.intra.peff.net","threadId":"56411","inReplyTo":"ed71913886e19ccc276b382de707b4bab7834d12.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-28T22:29:17Z","receivedAt":"2021-09-28T22:29:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 02:26:35AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> There are several considerations here:\n> * We have to pick file(s) where we write these conflict messages too\n> * We want to make it as clear as possible that it's not a real file\n> but a set of messages about another file\n> * We want conflict messages about a file to appear near the file in\n> question in a diff, preferably immediately preceding the file in\n> question\n> * Related to the above, per-file conflict messages are preferred\n> over lumping all conflict messages into one big file\n> \n> To achive the above:\n> * We put the conflict messages for $filename in\n> $filename[0:-1] + \" \" + $filename[-1] + \".conflict_msg\"\n> or, in words, we insert a space before the final character of\n> the filename and then also add \".conflict_msg\" at the end.\n\nIt took me a minute to understand the space thing. I thought at first it\nwas about avoiding conflicts with existing names (and while it might\nhelp in practice, it's not a guarantee). But I think it's about the\n\"appear preceding the file\" goal. The space sorts before any other\nprintable character in the final position.\n\nThat's...simultaneously clever and gross. My biggest complaint is that\nthe space looks like a bug in the output.\n\nUsing another character like \".\" might not be too bad, as it's also\nfairly early in the ascii table. But it's really this \"do it before the\nlast character\" thing that is key to getting the ordering right.\n\nJust brainstorming some alternatives:\n\n - we have diff.orderFile, etc. Could we stuff this data into a less\n confusing name (even just \"$filename.conflict_msg\"), and then provide\n a custom ordering to the diff code? I think it could be done by\n generating a static ordering ahead of time, but it might even just be\n possible to tell diffcore_order() to take the \".conflict_msg\"\n extension into account in its comparison function.\n\n - there can be other non-diff data between the individual segments. For\n example, \"patch\" will skip over non-diff lines. And certainly in Git\n we have our own custom headers. I'm wondering if we could attach\n these annotations to the diff-pair somehow, and then show something\n like:\n\n diff --git a/foo.c b/foo.c\n index 1234abcd..5678cdef 100644\n conflict modify/delete foo.c\n --- a/foo.c\n +++ b/foo.c\n @@ some actual diff starts here @@\n\nObviously such a thing can't really be applied. But then you wouldn't\nwant to apply the addition of \"my.file e.conflict_msg\" either.\n\nI dunno. The latter especially is definitely more work, and requires a\nbit more cooperation between the merge and diff code. In particular, you\ncan't just feed a straight tree to the diff anymore. We have to hold\nback the annotations, and then apply them to the resulting diff. But I\nthink the output is much more pleasing to the eye.\n\n-Peff\n"},{"id":"437373","messageId":"YVOZRhWttzF18Xql@coredump.intra.peff.net","threadId":"56411","inReplyTo":"2bf5efb5169b71ee7c1da006a74aa67794f1399d.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/7] ll-merge: add API for capturing warnings in a strbuf instead of stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-28T22:37:58Z","receivedAt":"2021-09-28T22:38:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n> \n> Whenever ll-merge encounters a binary file, it prints a warning to\n> stderr. However, for the --remerge-diff option we want to add, we need\n> to capture all conflict messages and show them in a diff instead of\n> dumping them straight to stdout or stderr. Add some new API that will\n> allow us to capture this output and display it in our preferred method.\n\nThis is a reasonable strategy for error-handling in general (though some\nmore thoughts below).\n\n> @@ -71,8 +75,11 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n> \t} else {\n> \t\tswitch (opts->variant) {\n> \t\tdefault:\n> -\t\t\twarning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> -\t\t\t\tpath, name1, name2);\n> +\t\t\tif (warnings) {\n> +\t\t\t\tstrbuf_addstr(warnings, \"Warning: \");\n> +\t\t\t\tstrbuf_addf(warnings, msg, path, name1, name2);\n> +\t\t\t} else\n> +\t\t\t\twarning(msg, path, name1, name2);\n\nThe usual warn_builtin() has a lowercase \"warning: \" prefix, but your\nstrbuf variant uses uppercase.\n\n> @@ -98,6 +105,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n> \n> static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n> \t\t\tmmbuffer_t *result,\n> +\t\t\tstruct strbuf *warnings,\n> \t\t\tconst char *path,\n> \t\t\tmmfile_t *orig, const char *orig_name,\n> \t\t\tmmfile_t *src1, const char *name1,\n\nThere's a lot of plumbing this variable through. This is probably too\ngross, but another option would be to call set_warn_routine() to\noverride it temporarily. It's gross because it affects everything, not\njust this call stack (and I also think we'd need to beef up the warn\nroutine code to handle some of the rough edges).\n\nI do wonder if the ll_merge() code should avoid calling warning() in the\nfirst place. It is after all, meant to be \"low-level\". We already return\nan error code from the function. I wonder if returning a more detailed\ncode instead, like:\n\n enum LL_MERGE_RESULT {\n\tLL_MERGE_OK = 0,\n\tLL_MERGE_CONFLICT,\n\tLL_MERGE_BINARY_CONFLICT,\n };\n\nwould let the caller do the sensible thing.\n\n-Peff\n"},{"id":"437374","messageId":"YVOZty9D7NRbzhE5@coredump.intra.peff.net","threadId":"56411","inReplyTo":"ce95b82fc492d48fa6022df424f9a303a1c70ad4.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/7] merge-ort: capture and print ll-merge warnings in our preferred fashion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-28T22:39:51Z","receivedAt":"2021-09-28T22:39:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 02:26:37AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n> \n> Instead of immediately printing ll-merge warnings to stderr, we save\n> them in our output strbuf. Besides allowing us to move these warnings\n> to a special file for --remerge-diff, this has two other benefits for\n> regular merges done by merge-ort:\n> \n> * The deferral of messages ensures we can print all messages about\n> any given path together (merge-recursive was known to sometimes\n> intersperse messages about other paths, particularly when renames\n> were involved).\n> \n> * The deferral of messages means we can avoid printing spurious\n> conflict messages when we just end up aborting due to local user\n> modifications in the way. (In contrast to merge-recursive.c which\n> prematurely checks for local modifications in the way via\n> unpack_trees() and gets the check wrong both in terms of false\n> positives and false negatives relative to renames, merge-ort does\n> not perform the local modifications in the way check until the\n> checkout() step after the full merge has been computed.)\n\nYeah, this is a good example of why having ll_merge() print warnings in\nthe first place is probably wrong. If you buy my argument that\nll_merge() should be returning an enum, then this code becomes IMHO even\nnicer, as you can generate your own sensible message in the caller.\n\n-Peff\n"},{"id":"437376","messageId":"YVOiggCWAdZcxAb6@coredump.intra.peff.net","threadId":"56411","inReplyTo":"67d3b2b09f9ddda616cdd0d1b12ab7afc73670ed.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-28T23:17:22Z","receivedAt":"2021-09-28T23:17:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 31, 2021 at 02:26:38AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n> \n> The tmp_objdir API provides the ability to create temporary object\n> directories, but was designed with the goal of having subprocesses\n> access these object stores, followed by the main process migrating\n> objects from it to the main object store or just deleting it. The\n> subprocesses would view it as their primary datastore and write to it.\n> \n> For the --remerge-diff option we want to add to show & log, we want all\n> writes of intermediate merge results (both blobs and trees) to go to\n> this alternate object store; since those writes will be done by the main\n> process, we need this \"alternate\" object store to actually be the\n> primary object store. When show & log are done, they'll simply remove\n> this temporary object store.\n\nI think this is consistent with the original design of tmp_objdir. I\njust never needed to do anything in-process before, so overriding the\nenvironment of sub-processes was sufficient.\n\nI do think these are dangerous and may cause bugs, though. Anything you\nwrite while the tmp_objdir is marked as \"primary\" is going to go away\nwhen you remove it. So any object names you reference outside of that,\neither:\n\n - by another object that you create after the tmp_objdir is removed;\n or\n\n - in a ref\n\nare going to turn into repository corruption. Of course that's true for\nthe existing sub-processes, too, but here we're touching a wider variety\nof code.\n\nObviously the objects we write as part of remerge-diff are meant to be\ntemporary, and we'll never reference them in any other way. And usually\nwe would not expect a diff to write any other objects. But one thing\nthat comes to mind if textconv caching.\n\nIf you do a remerge diff on a blob that uses textconv, and the caching\nfeature is turned on, then we'll write out a new blob with the cached\nvalue, and eventually a new tree and refs/notes/ pointer referencing it.\nI'm not sure of the timing of all of that, but it seems likely to me\nthat at least some of that will end up in your tmp_objdir.\n\nIf you remove the tmp_objdir as the primary as soon as you're done with\nthe merge, but before you run the diff, you might be OK, though.\n\nIf not, then I think the solution is probably not to install this as the\n\"primary\", but rather:\n\n - do the specific remerge-diff writes we want using a special \"write\n to this object dir\" variant of write_object_file()\n\n - install the tmp_objdir as an alternate, so the reading side (which\n is diff code that doesn't otherwise know about our remerge-diff\n trickery) can find them\n\nAnd that lets other writers avoid writing into the temporary area\naccidentally.\n\nIn that sense this is kind of like the pretend_object_file() interface,\nexcept that it's storing the objects on disk instead of in memory. Of\ncourse another way of doing that would be to stuff the object data into\ntempfiles and just put pointers into the in-memory cached_objects array.\n\nIt's also not entirely foolproof (nor is the existing\npretend_object_file()). Any operation which is fooled into thinking we\nhave object X because it's in the fake-object list or the tmp_objdir may\nreference that object erroneously, creating a corruption. But it's still\nsafer than allowing arbitrary writes into the tmp_objdir.\n\n Side note: The pretend_object_file() approach is actually even better,\n because we know the object is fake. So it does not confuse\n write_object_file()'s \"do we already have this object\" freshening\n check.\n\n I suspect it could even be made faster than the tmp_objdir approach.\n From our perspective, these objects really are tempfiles. So we could\n write them as such, not worrying about things like fsyncing them,\n naming them into place, etc. We could just write them out, then mmap\n the results, and put the pointers into cached_objects (currently it\n insists on malloc-ing a copy of the input buffer, but that seems like\n an easy extension to add).\n\n In fact, I think you could get away with just _one_ tempfile per\n merge. Open up one tempfile. Write out all of the objects you want to\n \"store\" into it in sequence, and record the lseek() offsets before and\n after for each object. Then mmap the whole result, and stuff the\n appropriate pointers (based on arithmetic with the offsets) into the\n cached_objects list.\n\n As a bonus, this tempfile can easily be in $TMPDIR, meaning\n remerge-diff could work on a read-only repository (and the OS can\n perhaps handle the data better, especially if $TMPDIR isn't a regular\n filesystem).\n\n> We also need one more thing -- `git log --remerge-diff` can cause the\n> temporary object store to fill up with loose objects. Rather than\n> trying to gc that are known garbage anyway, we simply want to know the\n> location of the temporary object store so we can purge the loose objects\n> after each merge.\n\nThis paragraph confused me. At first I thought you were talking about\nhow to avoid calling \"gc --auto\" because we don't want to waste time\nthinking all those loose objects were worth gc-ing. But we wouldn't do\nthat anyway (git-log does not expect to make objects so doesn't call it\nat all, and anyway you'd expect it to happen at the end of a process,\nafter we've already removed the tmp_objdir).\n\nBut I think you just mean: we can build up a bunch of loose objects,\nwhich is inefficient. We don't want to gc them, because that's even less\nefficient. So we want to clean them out between merges.\n\nBut I don't see any code to do that here. I guess that's maybe why\nyou've added tmp_objdir_path(), to find them later. But IMHO this would\nbe much better encapsulated as tmp_objdir_clear_objects() or something.\n\nBut simpler still: is there any reason not to just drop and re-create\nthe tmp-objdir for each merge? It's not very expensive to do so (and\ncertainly not compared to the cost of actually writing out the objects).\n\n-Peff\n"},{"id":"437393","messageId":"xmqqpmssgt0d.fsf@gitster.g","threadId":"56411","inReplyTo":"YVOZRhWttzF18Xql@coredump.intra.peff.net","subject":"Re: [PATCH 3/7] ll-merge: add API for capturing warnings in a strbuf instead of stderr","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-28T23:49:54Z","receivedAt":"2021-09-28T23:50:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do wonder if the ll_merge() code should avoid calling warning() in the\n> first place. It is after all, meant to be \"low-level\". We already return\n> an error code from the function. I wonder if returning a more detailed\n> code instead, like:\n>\n> enum LL_MERGE_RESULT {\n> \tLL_MERGE_OK = 0,\n> \tLL_MERGE_CONFLICT,\n> \tLL_MERGE_BINARY_CONFLICT,\n> };\n>\n> would let the caller do the sensible thing.\n\nI like it better as a longer-term direction.\n\n"},{"id":"437426","messageId":"CABPp-BGwunfsThNEPAKfXMUZKpHQTCdRU7V8=3J88XThN6=2Kg@mail.gmail.com","threadId":"56411","inReplyTo":"87mtnxxgz3.fsf@evledraar.gmail.com","subject":"Re: [PATCH 6/7] show, log: provide a --remerge-diff capability","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-29T04:00:12Z","receivedAt":"2021-09-29T04:00:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 28, 2021 at 1:05 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > static int decoration_given;\n> > static int use_mailmap_config = 1;\n> > +static struct tmp_objdir *tmp_objdir;\n> > static const char *fmt_patch_subject_prefix = \"PATCH\";\n> > static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;\n> > static const char *fmt_pretty;\n>\n> So here we make this static file-level etc...\n>\n> > @@ -407,6 +410,17 @@ static int cmd_log_walk(struct rev_info *rev)\n> > int saved_nrl = 0;\n> > int saved_dcctc = 0;\n> >\n> > + if (rev->remerge_diff) {\n> > + tmp_objdir = tmp_objdir_create();\n> > + if (!tmp_objdir)\n> > + die(_(\"unable to create temporary object directory\"));\n> > + tmp_objdir_make_primary(the_repository, tmp_objdir);\n> > +\n> > + strbuf_init(&rev->remerge_objdir_location, 0);\n> > + strbuf_addstr(&rev->remerge_objdir_location,\n> > + tmp_objdir_path(tmp_objdir));\n> > + }\n> > +\n> > if (rev->early_output)\n> > setup_early_output();\n> >\n> > @@ -449,6 +463,13 @@ static int cmd_log_walk(struct rev_info *rev)\n> > rev->diffopt.no_free = 0;\n> > diff_free(&rev->diffopt);\n> >\n> > + if (rev->remerge_diff) {\n> > + strbuf_release(&rev->remerge_objdir_location);\n> > + tmp_objdir_remove_as_primary(the_repository, tmp_objdir);\n> > + tmp_objdir_destroy(tmp_objdir);\n> > + tmp_objdir = NULL;\n>\n> ...but all of the \"tmp_objdir\" usage is in one function, can't the\n> variable be declared here instead?\n\nThat's a very good point.\n\n> We need to hand the \"remerge_objdir_location\" off to the \"rev_info\"\n> struct, but that seems separate from its lifetime.\n\nGiven Peff's suggestion elsewhere, though, to destroy the tmp_objdir\nafter each merge and create a new one, I wonder if I should actually\nbe passing a tmp_objdir** to rev_info (allowing log-tree to do the\nwork of destroying and creating a new one after each merge, instead of\nusing the \"remerge_objdir_location\" to run a recursive delete of\nfiles). That'd still work with your idea to remove the statically\nscoped variable, though.\n\n> Re my [1] & [2] I like Neeraj's \"atexit cleanup\" approach better,\n> perhaps that makes your cleanup in log-tree.c redundant or easier?\n\nHaving an atexit cleanup as a safety measure seems fine. However, I\ndon't like avoiding the manual cleanup step and relying on atexit\ncleanup; I'd go so far as to say I think that'd be a bug, at least for\nmy usage. It presumes one-shot usage, whereas I'd rather move git to\nbeing more library-like.\n\nHowever, fully destroying the tmp_objdir probably makes the cleanup in\nlog-tree.c easier.\n\n> Per [2] it looks like you need to \"hand off\" the\n> \"remerge_objdir_location\", so having the struct live in tmp-objdir.h as\n> I suggested in [2] might make that work...\n>\n> 1. https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/\n> 2. https://lore.kernel.org/git/87r1d9xh71.fsf@evledraar.gmail.com/\n"},{"id":"437427","messageId":"CABPp-BGH+zgLuxQpy8DF0MefGfoVc26Z2RwsNN9yvs98jvkn3Q@mail.gmail.com","threadId":"56411","inReplyTo":"YVOZRhWttzF18Xql@coredump.intra.peff.net","subject":"Re: [PATCH 3/7] ll-merge: add API for capturing warnings in a strbuf instead of stderr","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-29T04:03:56Z","receivedAt":"2021-09-29T04:04:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 28, 2021 at 3:37 PM Jeff King wrote:\n>\n> On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > Whenever ll-merge encounters a binary file, it prints a warning to\n> > stderr. However, for the --remerge-diff option we want to add, we need\n> > to capture all conflict messages and show them in a diff instead of\n> > dumping them straight to stdout or stderr. Add some new API that will\n> > allow us to capture this output and display it in our preferred method.\n>\n> This is a reasonable strategy for error-handling in general (though some\n> more thoughts below).\n>\n> > @@ -71,8 +75,11 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n> > } else {\n> > switch (opts->variant) {\n> > default:\n> > - warning(\"Cannot merge binary files: %s (%s vs. %s)\",\n> > - path, name1, name2);\n> > + if (warnings) {\n> > + strbuf_addstr(warnings, \"Warning: \");\n> > + strbuf_addf(warnings, msg, path, name1, name2);\n> > + } else\n> > + warning(msg, path, name1, name2);\n>\n> The usual warn_builtin() has a lowercase \"warning: \" prefix, but your\n> strbuf variant uses uppercase.\n\nAh, thanks for catching that. I can fix it up.\n\n> > @@ -98,6 +105,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,\n> >\n> > static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n> > mmbuffer_t *result,\n> > + struct strbuf *warnings,\n> > const char *path,\n> > mmfile_t *orig, const char *orig_name,\n> > mmfile_t *src1, const char *name1,\n>\n> There's a lot of plumbing this variable through. This is probably too\n> gross, but another option would be to call set_warn_routine() to\n> override it temporarily. It's gross because it affects everything, not\n> just this call stack (and I also think we'd need to beef up the warn\n> routine code to handle some of the rough edges).\n>\n> I do wonder if the ll_merge() code should avoid calling warning() in the\n> first place. It is after all, meant to be \"low-level\". We already return\n> an error code from the function. I wonder if returning a more detailed\n> code instead, like:\n>\n> enum LL_MERGE_RESULT {\n> LL_MERGE_OK = 0,\n> LL_MERGE_CONFLICT,\n> LL_MERGE_BINARY_CONFLICT,\n> };\n>\n> would let the caller do the sensible thing.\n\nOoh, I like this idea. I'll have to change all the callers, but\nthere's only about half a dozen or so...\n"},{"id":"437428","messageId":"xmqqsfxof2hr.fsf@gitster.g","threadId":"56411","inReplyTo":"YVOiggCWAdZcxAb6@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T04:08:00Z","receivedAt":"2021-09-29T04:08:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Side note: The pretend_object_file() approach is actually even better,\n> because we know the object is fake. So it does not confuse\n> write_object_file()'s \"do we already have this object\" freshening\n> check.\n>\n> I suspect it could even be made faster than the tmp_objdir approach.\n> From our perspective, these objects really are tempfiles. So we could\n> write them as such, not worrying about things like fsyncing them,\n> naming them into place, etc. We could just write them out, then mmap\n> the results, and put the pointers into cached_objects (currently it\n> insists on malloc-ing a copy of the input buffer, but that seems like\n> an easy extension to add).\n>\n> In fact, I think you could get away with just _one_ tempfile per\n> merge. Open up one tempfile. Write out all of the objects you want to\n> \"store\" into it in sequence, and record the lseek() offsets before and\n> after for each object. Then mmap the whole result, and stuff the\n> appropriate pointers (based on arithmetic with the offsets) into the\n> cached_objects list.\n\nCute. The remerge diff code path creates a full tree that records\nthe mechanical merge result. By hooking into the lowest layer of\nwrite_object() interface, we'd serialize all objects in such a tree\nin the order they are computed (bottom up from the leaf level, I'd\npresume) into a single flat file ;-)\n"},{"id":"437430","messageId":"CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com","threadId":"56411","inReplyTo":"87r1d9xh71.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-29T04:22:32Z","receivedAt":"2021-09-29T04:22:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 28, 2021 at 1:00 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n>\n> I commented just now on how this API is duplicated between here &\n> another in-flight series in\n> https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/; Just\n> comments on this patch here:\n>\n> > diff --git a/tmp-objdir.c b/tmp-objdir.c\n> > index b8d880e3626..9f75a75d1c0 100644\n> > --- a/tmp-objdir.c\n> > +++ b/tmp-objdir.c\n> > @@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)\n> > return t->env.v;\n> > }\n> >\n> > +const char *tmp_objdir_path(struct tmp_objdir *t)\n> > +{\n> > + return t->path.buf;\n> > +}\n>\n> Not your fault & this pre-dates your patch, but FWIW I prefer our APIs\n> that don't have these \"hidden struct\" shenanigans (like say \"struct\n> string_list\") so a caller could just access this, we can just declare it\n> \"const\" appropriately.\n>\n> We're also all in-tree friends here, so having various accessors for no\n> reason other than to access struct members seems a bit too much.\n\nThat's a fair point, but just to play counterpoint for a minute...\n\nFWIW, I dislike when our public facing APIs are polluted with all\nkinds of internal details. merge-recursive being a case in point.\nWhen writing merge-ort, although I had a struct with public fields,\nthat struct also contained an opaque struct (pointer) within it to\nhide several private fields. (I would have liked to hide or remove a\nfew more fields, but couldn't do so while the merge_recursive_options\nstruct was shared between both merge-ort and merge-recursive.)\n\nThat said, I agree it can certainly be overdone, and tmp_objdir is\npretty simple. However, sometimes even in simple cases I like when\nfolks make use of an opaque struct because it means folks put some\neffort into thinking more about the API that should be exposed.\nThat's something we as a project have often overlooked in the past, as\nevidenced both by our one-shot usage mentality, and the existence of\nexternal projects like libgit2 attempting to correct this design\nshortcoming. I'd like git to move more towards being structured as a\nreusable library as well as a useful command-line tool.\n\n> All of which is to say if you + Neeraj come up with some common API I\n> for one wouldn't mind moving the struct deceleration to tmp-objdir.h,\n> but it looks like in his version it can stay \"private\" more easily.\n\nPeff's ideas to just delete and recreate the temporary object store\nafter every merge would let me reduce the API extensions and keep\nthings more private. So I think our ideas are converging a bit. :-)\n\n> In this particular case I hadn't understood on a previous reading of\n> tmp-objdir.c why this \"path\" isn't a statically allocated \"char\n> path[PATH_MAX]\", or a least we that hardcoded \"1024\" should be a\n> PATH_MAX, as it is in some other cases.\n\nActually, PATH_MAX shouldn't be used:\n\nhttps://lore.kernel.org/git/7d1237c7-5a83-d766-7d93-5f0d59166067@web.de/\nhttps://lore.kernel.org/git/20180720180427.GE22486@sigill.intra.peff.net/\n"},{"id":"437432","messageId":"CABPp-BFuj3W20J-aKU4BL9cqgA-GPZ2prEbAebY_kR0adaGzHw@mail.gmail.com","threadId":"56411","inReplyTo":"YVOiggCWAdZcxAb6@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-29T05:05:31Z","receivedAt":"2021-09-29T05:05:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 28, 2021 at 4:17 PM Jeff King wrote:\n>\n> On Tue, Aug 31, 2021 at 02:26:38AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > The tmp_objdir API provides the ability to create temporary object\n> > directories, but was designed with the goal of having subprocesses\n> > access these object stores, followed by the main process migrating\n> > objects from it to the main object store or just deleting it. The\n> > subprocesses would view it as their primary datastore and write to it.\n> >\n> > For the --remerge-diff option we want to add to show & log, we want all\n> > writes of intermediate merge results (both blobs and trees) to go to\n> > this alternate object store; since those writes will be done by the main\n> > process, we need this \"alternate\" object store to actually be the\n> > primary object store. When show & log are done, they'll simply remove\n> > this temporary object store.\n>\n> I think this is consistent with the original design of tmp_objdir. I\n> just never needed to do anything in-process before, so overriding the\n> environment of sub-processes was sufficient.\n>\n> I do think these are dangerous and may cause bugs, though. Anything you\n> write while the tmp_objdir is marked as \"primary\" is going to go away\n> when you remove it. So any object names you reference outside of that,\n> either:\n>\n> - by another object that you create after the tmp_objdir is removed;\n> or\n>\n> - in a ref\n>\n> are going to turn into repository corruption. Of course that's true for\n> the existing sub-processes, too, but here we're touching a wider variety\n> of code.\n>\n> Obviously the objects we write as part of remerge-diff are meant to be\n> temporary, and we'll never reference them in any other way. And usually\n> we would not expect a diff to write any other objects. But one thing\n> that comes to mind if textconv caching.\n>\n> If you do a remerge diff on a blob that uses textconv, and the caching\n> feature is turned on, then we'll write out a new blob with the cached\n> value, and eventually a new tree and refs/notes/ pointer referencing it.\n> I'm not sure of the timing of all of that, but it seems likely to me\n> that at least some of that will end up in your tmp_objdir.\n\nInteresting. Thanks for explaining this case, and for the testcase in\nthe other thread.\n\nSo there are other parts of running `git log` that are not read-only,\nbesides what I'm doing. That's unfortunate. Can we just disable\nthose things in this section of the code?\n\nAlso, I think it makes sense that textconv caching causes a problem\nvia writing new blobs and trees *and* refs that reference these blobs\nand trees. However, I'm not sure I'm really grasping it, because I\ndon't see how your solutions are safe either. I'll mention more after\neach one.\n\n> If you remove the tmp_objdir as the primary as soon as you're done with\n> the merge, but before you run the diff, you might be OK, though.\n\nIt has to be after I run the diff, because the diff needs access to\nthe temporary files to diff against them.\n\n> If not, then I think the solution is probably not to install this as the\n> \"primary\", but rather:\n>\n> - do the specific remerge-diff writes we want using a special \"write\n> to this object dir\" variant of write_object_file()\n>\n> - install the tmp_objdir as an alternate, so the reading side (which\n> is diff code that doesn't otherwise know about our remerge-diff\n> trickery) can find them\n>\n> And that lets other writers avoid writing into the temporary area\n> accidentally.\n\nDoesn't this have the same problem? If something similar to textconv\ncaching were to create new trees that referenced the existing blobs\nand then added a ref that referred to that tree, then you have the\nexact same problem, right? The new tree and the new ref would be\ncorrupt as soon as the tmp-objdir went away. Whether or not other\ncallers write to the temporary area isn't the issue, it's whether they\nwrite refs that can refer to anything from the temporary area. Or am\nI missing something?\n\n> In that sense this is kind of like the pretend_object_file() interface,\n> except that it's storing the objects on disk instead of in memory. Of\n> course another way of doing that would be to stuff the object data into\n> tempfiles and just put pointers into the in-memory cached_objects array.\n>\n> It's also not entirely foolproof (nor is the existing\n> pretend_object_file()). Any operation which is fooled into thinking we\n> have object X because it's in the fake-object list or the tmp_objdir may\n> reference that object erroneously, creating a corruption. But it's still\n> safer than allowing arbitrary writes into the tmp_objdir.\n\nWhy is the pretend_object_file() interface safer? Does it disable the\ntextconv caching somehow? I don't see why it helps avoid this\nproblem.\n\n> Side note: The pretend_object_file() approach is actually even better,\n> because we know the object is fake. So it does not confuse\n> write_object_file()'s \"do we already have this object\" freshening\n> check.\n\nOh, that's interesting. That would be helpful.\n\n> I suspect it could even be made faster than the tmp_objdir approach.\n> From our perspective, these objects really are tempfiles. So we could\n> write them as such, not worrying about things like fsyncing them,\n> naming them into place, etc. We could just write them out, then mmap\n> the results, and put the pointers into cached_objects (currently it\n> insists on malloc-ing a copy of the input buffer, but that seems like\n> an easy extension to add).\n>\n> In fact, I think you could get away with just _one_ tempfile per\n> merge. Open up one tempfile. Write out all of the objects you want to\n> \"store\" into it in sequence, and record the lseek() offsets before and\n> after for each object. Then mmap the whole result, and stuff the\n> appropriate pointers (based on arithmetic with the offsets) into the\n> cached_objects list.\n>\n> As a bonus, this tempfile can easily be in $TMPDIR, meaning\n> remerge-diff could work on a read-only repository (and the OS can\n> perhaps handle the data better, especially if $TMPDIR isn't a regular\n> filesystem).\n\nInteresting. I'm not familiar with the pretend_object_file() code,\nbut I think I catch what you're saying. I don't see anything in\nobject-file.c that allows removing items from the cache, as I would\nwant to do, but I'm sure I could add something.\n\n(I'm still not sure how this avoids the problem of things like\ntextconv caching trying to write an object that references one of\nthese, though. Should we just manually disable textconv caching\nduring a remerge-diff?)\n\n> > We also need one more thing -- `git log --remerge-diff` can cause the\n> > temporary object store to fill up with loose objects. Rather than\n> > trying to gc that are known garbage anyway, we simply want to know the\n> > location of the temporary object store so we can purge the loose objects\n> > after each merge.\n>\n> This paragraph confused me. At first I thought you were talking about\n> how to avoid calling \"gc --auto\" because we don't want to waste time\n> thinking all those loose objects were worth gc-ing. But we wouldn't do\n> that anyway (git-log does not expect to make objects so doesn't call it\n> at all, and anyway you'd expect it to happen at the end of a process,\n> after we've already removed the tmp_objdir).\n>\n> But I think you just mean: we can build up a bunch of loose objects,\n> which is inefficient. We don't want to gc them, because that's even less\n> efficient. So we want to clean them out between merges.\n\nYes, sorry that wasn't clear.\n\n> But I don't see any code to do that here. I guess that's maybe why\n> you've added tmp_objdir_path(), to find them later. But IMHO this would\n> be much better encapsulated as tmp_objdir_clear_objects() or something.\n>\n> But simpler still: is there any reason not to just drop and re-create\n> the tmp-objdir for each merge? It's not very expensive to do so (and\n> certainly not compared to the cost of actually writing out the objects).\n\nOoh, indeed. I like that, and it turns out that the majority of the\ncost of dropping the tmp_objdir is the recursive directory removal,\nthat I wanted anyway as part of me inbetween-merges cleanup.\n\nBut, of course, if we go the pretend_object_file() route then the\ntmp_objdir() stuff probably becomes moot.\n"},{"id":"437436","messageId":"CABPp-BEioOUJRd6FSsmsDtYHhUy7xhr4YabdEmVKzkduo4g9TQ@mail.gmail.com","threadId":"56411","inReplyTo":"YVOXPTjsp9lrxmS6@coredump.intra.peff.net","subject":"Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-29T06:25:20Z","receivedAt":"2021-09-29T06:25:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Sep 28, 2021 at 3:29 PM Jeff King wrote:\n>\n> On Tue, Aug 31, 2021 at 02:26:35AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > There are several considerations here:\n> > * We have to pick file(s) where we write these conflict messages too\n> > * We want to make it as clear as possible that it's not a real file\n> > but a set of messages about another file\n> > * We want conflict messages about a file to appear near the file in\n> > question in a diff, preferably immediately preceding the file in\n> > question\n> > * Related to the above, per-file conflict messages are preferred\n> > over lumping all conflict messages into one big file\n> >\n> > To achive the above:\n> > * We put the conflict messages for $filename in\n> > $filename[0:-1] + \" \" + $filename[-1] + \".conflict_msg\"\n> > or, in words, we insert a space before the final character of\n> > the filename and then also add \".conflict_msg\" at the end.\n>\n> It took me a minute to understand the space thing. I thought at first it\n> was about avoiding conflicts with existing names (and while it might\n> help in practice, it's not a guarantee). But I think it's about the\n> \"appear preceding the file\" goal. The space sorts before any other\n> printable character in the final position.\n\nYeah, it's all about the ordering. I guess it helps slightly with\nconflict avoidance, but I cannot rely on it; I have to check for\ncolliding files and potentially tweak the filename further.\n\n> That's...simultaneously clever and gross. My biggest complaint is that\n> the space looks like a bug in the output.\n\nJunio had basically the same reaction[*]. :-)\n\n[*] https://lore.kernel.org/git/xmqqk0k0qkmv.fsf@gitster.g/\n\n> Using another character like \".\" might not be too bad, as it's also\n> fairly early in the ascii table. But it's really this \"do it before the\n> last character\" thing that is key to getting the ordering right.\n>\n> Just brainstorming some alternatives:\n>\n> - we have diff.orderFile, etc. Could we stuff this data into a less\n> confusing name (even just \"$filename.conflict_msg\"), and then provide\n> a custom ordering to the diff code? I think it could be done by\n> generating a static ordering ahead of time, but it might even just be\n> possible to tell diffcore_order() to take the \".conflict_msg\"\n> extension into account in its comparison function.\n\nI can't just go on the \".conflict_msg\" extension. As you noted above,\nthis scheme is not sufficient for avoiding collisions. So I need to\nappend extra \"cruft\" to the name in the case of collisions -- meaning\nwe can't special case on just that extension.\n\nI also don't like how diff.orderFile provides a global ordering of the\nfiles listed, rather than providing some scheme for relative\norderings. That'd either force me to precompute the diff to determine\nall the files that were different so I can list _all_ of them, or put\nup with the fact that the files with non-content conflicts will be\nlisted very first in the output, even if their name is\n'zee-last-file.c' -- surprising users at the output ordering.\n\nThis also means that if the user had their own ordering defined, then\nI'm overriding it and messing up their ordering, which might be\nproblematic.\n\nSo, I'm not so sure about this solution; it feels like it introduces\nbigger holes than the ugly space character it is fixing.\n\n> - there can be other non-diff data between the individual segments. For\n> example, \"patch\" will skip over non-diff lines. And certainly in Git\n> we have our own custom headers. I'm wondering if we could attach\n> these annotations to the diff-pair somehow, and then show something\n> like:\n>\n> diff --git a/foo.c b/foo.c\n> index 1234abcd..5678cdef 100644\n> conflict modify/delete foo.c\n\nA couple things here...\n\nFirst, I'm not so sure I like the abbreviation here. Just knowing\n\"modify/delete\" might be enough in some cases, but I'd rather have the\nfull messages that would have been printed to the console, e.g.:\n\nCONFLICT (modify/delete): foo.c deleted in HASH1 (SHORT\nSUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2\n(SHORT SUMMARY2) of foo.c left in tree.\n\nbecause I think the commit references are useful context. That extra\ncontext might be of little use for many modify/delete conflicts, but\nis much more important for conflicts involving renames; e.g.\n\"rename/rename\" is much less useful than being able to know the\noriginal name of the file and with which parent commit each filename\nis associated. So, that raises the question: could we pack all that\ninformation from the full conflict notice into these conflict\nheader(s)? (And do we have to special case the code to print it all\non one line when doing the remerge-diff since the diff output needs\nthem to be one-line headers?)\n\nSecond, what about when there are multiple non-content conflict types\nfor a single file, e.g. rename/delete + rename/add + modify/delete +\nmode conflict + unmergeable binary? (Yes, I think it's possible for\none path to have all five of those: (1) source file deleted on one\nside, renamed on the other, (2) rename target on one side matches new\nfile added on other side of history, (3) renamed file also had its\ncontents modified, thus modify vs. delete, (4) added file on other\nside of history had a different mode, (5) added file on other side of\nhistory is a binary.) Do we just use multiple conflict headers?\n\nThird, what about the cases where there is no diff, just conflict\nheaders? (I suspect many modify/delete or rename/delete or binary\nfiles may end up in such a situation.)\n\nI don't think any of those are deal breakers, but it means more work,\nand maybe also other forms of ugliness.\n\n> --- a/foo.c\n> +++ b/foo.c\n> @@ some actual diff starts here @@\n>\n> Obviously such a thing can't really be applied. But then you wouldn't\n> want to apply the addition of \"my.file e.conflict_msg\" either.\n\nNit: \"my.fil e.conflict_msg\", not \"my.file e.conflict_msg\" (the 'e' in\n'file' is not repeated, otherwise the auxiliary file wouldn't sort\nbefore its companion file)\n\n> I dunno. The latter especially is definitely more work, and requires a\n> bit more cooperation between the merge and diff code. In particular, you\n> can't just feed a straight tree to the diff anymore. We have to hold\n> back the annotations, and then apply them to the resulting diff. But I\n> think the output is much more pleasing to the eye.\n\nIt's certainly an interesting idea. It's a lot more work, it involves\nthe inability to feed a straight tree to a diff would require piping\nthings through several different layers (merge -> log -> diff, and\npossible multiple diff layers), it may mean we need special handling\nfor when there are only conflict headers for a file with no file\ndifferences, the length of the conflict headers could be comically\nlong, and it's all essentially for what is a rather uncommon case\nanyway. But, on the plus side, it does avoid the rather ugly space.\n\nI'll have to think about it.\n"},{"id":"437465","messageId":"xmqqr1d7e4ug.fsf@gitster.g","threadId":"56411","inReplyTo":"CABPp-BEioOUJRd6FSsmsDtYHhUy7xhr4YabdEmVKzkduo4g9TQ@mail.gmail.com","subject":"Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-29T16:14:47Z","receivedAt":"2021-09-29T16:14:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I also don't like how diff.orderFile provides a global ordering of the\n> files listed, rather than providing some scheme for relative\n> orderings. That'd either force me to precompute the diff to determine\n> all the files that were different so I can list _all_ of them,...\n\nDon't we determine all the files that would be listed in the diff\nanyway? The diffcore pipeline collects all the different filepairs,\nmatches them up for rename/copy detection, and finally do the output\nformatting for each individual filepair.\n\n> So, I'm not so sure about this solution; it feels like it introduces\n> bigger holes than the ugly space character it is fixing.\n>\n>> - there can be other non-diff data between the individual segments. For\n>> example, \"patch\" will skip over non-diff lines. And certainly in Git\n>> we have our own custom headers. I'm wondering if we could attach\n>> these annotations to the diff-pair somehow, and then show something\n>> like:\n>>\n>> diff --git a/foo.c b/foo.c\n>> index 1234abcd..5678cdef 100644\n>> conflict modify/delete foo.c\n>\n> A couple things here...\n>\n> First, I'm not so sure I like the abbreviation here. Just knowing\n> \"modify/delete\" might be enough in some cases, but I'd rather have the\n> full messages that would have been printed to the console...\n>\n> Second, what about when there are multiple ...\n>\n> Third, what about the cases where there is no diff, ...\n\nNone of the above seems like a problem to me at least from the\npresentation and consumption sides. There is no rule that extended\ndiff headers has to fit on a 72-char line, cannot use line folding\nby inserting LF-SP in the middle of a logical line, and there\nalready is at least one case we give an extended diff header without\na single line of content change (namely, \"chmod +x README\").\n\nThanks.\n"},{"id":"437466","messageId":"CABPp-BEidvD6Hi0rj4tnG5xioi1RXQK95NjGJkPYqjqmaU12nA@mail.gmail.com","threadId":"56411","inReplyTo":"xmqqr1d7e4ug.fsf@gitster.g","subject":"Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-09-29T16:31:46Z","receivedAt":"2021-09-29T16:32:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Sep 29, 2021 at 9:14 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > I also don't like how diff.orderFile provides a global ordering of the\n> > files listed, rather than providing some scheme for relative\n> > orderings. That'd either force me to precompute the diff to determine\n> > all the files that were different so I can list _all_ of them,...\n>\n> Don't we determine all the files that would be listed in the diff\n> anyway? The diffcore pipeline collects all the different filepairs,\n> matches them up for rename/copy detection, and finally do the output\n> formatting for each individual filepair.\n\nmerge-ort is missing one side of the diff. It only has the parents of\nthe merge commit, and their merge base. So, merge-ort would now need\nto know about an additional commit (the original merge), and compute\nthe diff between that and the merge-result it is creating, in advance\nof an external caller (log) calling diff to compute the result between\nthose two trees.\n\n> > So, I'm not so sure about this solution; it feels like it introduces\n> > bigger holes than the ugly space character it is fixing.\n> >\n> >> - there can be other non-diff data between the individual segments. For\n> >> example, \"patch\" will skip over non-diff lines. And certainly in Git\n> >> we have our own custom headers. I'm wondering if we could attach\n> >> these annotations to the diff-pair somehow, and then show something\n> >> like:\n> >>\n> >> diff --git a/foo.c b/foo.c\n> >> index 1234abcd..5678cdef 100644\n> >> conflict modify/delete foo.c\n> >\n> > A couple things here...\n> >\n> > First, I'm not so sure I like the abbreviation here. Just knowing\n> > \"modify/delete\" might be enough in some cases, but I'd rather have the\n> > full messages that would have been printed to the console...\n> >\n> > Second, what about when there are multiple ...\n> >\n> > Third, what about the cases where there is no diff, ...\n>\n> None of the above seems like a problem to me at least from the\n> presentation and consumption sides. There is no rule that extended\n> diff headers has to fit on a 72-char line, cannot use line folding\n> by inserting LF-SP in the middle of a logical line, and there\n> already is at least one case we give an extended diff header without\n> a single line of content change (namely, \"chmod +x README\").\n\nCool, that's good to hear. I'll look into it.\n"},{"id":"437503","messageId":"YVVmssXlaFM6yD5W@coredump.intra.peff.net","threadId":"56411","inReplyTo":"CABPp-BFuj3W20J-aKU4BL9cqgA-GPZ2prEbAebY_kR0adaGzHw@mail.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-30T07:26:42Z","receivedAt":"2021-09-30T07:26:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 10:05:31PM -0700, Elijah Newren wrote:\n\n> So there are other parts of running `git log` that are not read-only,\n> besides what I'm doing. That's unfortunate. Can we just disable\n> those things in this section of the code?\n\nIt might be possible for the cache (we'd still generate entries, but\njust not write them). It's \"just\" an optimization, though it's possible\nthere are cases where it's really annoying to do so. I'd imagine those\nare a stretch. I do use textconv to decrypt gpg blobs with a key on a\nhardware token, for example. I don't require a touch for each one,\nthough, nor would I use textconv caching anyway (since the point is not\nto store the unencrypted contents).\n\nIt wouldn't necessarily be OK to disable object writes in general.\nThough I'd think since a diff is _conceptually_ a read-only operation\nthat any writes it wants to do would be in the same caching /\noptimization boat. I'd also worry about the maintenance cost of having\nto annotate any such case.\n\n> Also, I think it makes sense that textconv caching causes a problem\n> via writing new blobs and trees *and* refs that reference these blobs\n> and trees. However, I'm not sure I'm really grasping it, because I\n> don't see how your solutions are safe either. I'll mention more after\n> each one.\n\nHeh. So I originally wrote more on this in my earlier email, but worried\nI was getting too into the weeds and would confuse you, so I pared it\ndown. But I managed to be confusing anyway. :)\n\nHere's one mental model. If you have a temporary object store, then\nthese are the bad things that can happen:\n\n 1. Some code may write an object to it, thinking the data is saved,\n but we later throw it away.\n\n 2. Some code may _skip_ writing an object, thinking that we already\n have that object available (when in fact we'll later throw it\n away).\n\n 3. Some code may record the fact that we have an object in memory, and\n then later (after the temporary object store is not in use), may\n point to it when writing an object or reference.\n\nPointing the primary object-dir at a temporary directory (like your\npatch does) means we're subject to all three.\n\nProblem (1) happens because unrelated code may not realize we've swapped\nout the primary object writing code for this throw-away directory. But\nwe could require writers to explicitly indicate that they want to write\nto the temporary area, while allowing other writes to go to the regular\nobject store. This could be done with a flag or variant of\nwrite_object_file(). Or if the temporary area isn't a regular object\ndirectory at all (like the whole tempfile / pretend thing), then this\nhappens automatically, because the regular object-write paths don't even\nknow about our temporary area.\n\nProblem (2) is mostly handled inside write_object_file(). It will\noptimize out writes of objects we already have. So it needs to know when\nwe have an object \"for real\", and when we just have it in our temporary\nstore. If we make the tmp_objdir the primary, then it doesn't know this\n(though we could teach it to check. In the pretend_object_file() system,\nit already knows this (it uses the \"freshen\" code paths which actually\nwant to find the entry in the filesystem).\n\nProblem (3) is the hardest one, because we don't really distinguish\nbetween readers and writers. So if I call has_object_file(), is it\nbecause I want to access the object? Or is it because I want to generate\na new reference to it, which must ensure that it exists? One of those is\nOK to look at the tmp_objdir (and indeed, is the whole point of making\nit in the first place), and the other is the first step to corrupting\nthe repository. ;)\n\nWith the environment variables we set up for running external commands\n(like hooks) in the receive-pack quarantine, one bit of safety we have\nis to prevent ref writes entirely in those commands. That works\nreasonably well. Even if they may write new objects that might get\nthrown away (if the push is rejected), they'd ultimately need to point\nto those objects with a ref. And because those hooks spend their\n_entire_ run-time in the quarantine state, they can't ever write a ref.\n\nWhereas doing it in-process is a little dicier. Right now the textconv\ncache writes out a new tree for every entry we add, so it happens to do\nthe ref write while the tmp-objdir is still in place. But as this\ncomment notes:\n\n $ git grep -B6 notes_cache_write diff.c\n diff.c- /*\n diff.c- * we could save up changes and flush them all at the end,\n diff.c- * but we would need an extra call after all diffing is done.\n diff.c- * Since generating a cache entry is the slow path anyway,\n diff.c- * this extra overhead probably isn't a big deal.\n diff.c- */\n diff.c: notes_cache_write(textconv->cache);\n\nthis is slow, and it would be perfectly reasonable to flush the cache at\nthe end of the process (e.g., using an atexit() handler). In which case\nwe'd hit this problem (3) exactly: we'd generate and remember objects\nduring the tmp-objdir period, but only later actually reference them.\nThis is more likely than the receive-pack hook case, because we're doing\nit all in process.\n\n> > If you remove the tmp_objdir as the primary as soon as you're done with\n> > the merge, but before you run the diff, you might be OK, though.\n> \n> It has to be after I run the diff, because the diff needs access to\n> the temporary files to diff against them.\n\nRight, of course. I was too fixated on the object-write part, forgetting\nthat the whole point of the exercise is to later read them back. :)\n\n> > If not, then I think the solution is probably not to install this as the\n> > \"primary\", but rather:\n> >\n> > - do the specific remerge-diff writes we want using a special \"write\n> > to this object dir\" variant of write_object_file()\n> >\n> > - install the tmp_objdir as an alternate, so the reading side (which\n> > is diff code that doesn't otherwise know about our remerge-diff\n> > trickery) can find them\n> >\n> > And that lets other writers avoid writing into the temporary area\n> > accidentally.\n> \n> Doesn't this have the same problem? If something similar to textconv\n> caching were to create new trees that referenced the existing blobs\n> and then added a ref that referred to that tree, then you have the\n> exact same problem, right? The new tree and the new ref would be\n> corrupt as soon as the tmp-objdir went away. Whether or not other\n> callers write to the temporary area isn't the issue, it's whether they\n> write refs that can refer to anything from the temporary area. Or am\n> I missing something?\n\nThe key thing here is in the first step, where remerge-diff is\nexplicitly saying \"I want to write to this temporary object-dir\". But\nthe textconv-cache code does not; it gets to write to the normal spot.\nSo it avoids problem (1).\n\nYou're right that it does not avoid problem (3) exactly. But triggering\nthat would require some code not just writing other objects or\nreferences while the tmp-objdir is in place, but specifically\nreferencing the objects that remerge-diff did put into the tmp-objdir.\nThat seems a lot less likely to me (because the thing we're most worried\nabout is unrelated code that just happens to write while the tmp-objdir\nis in place).\n\n> > In that sense this is kind of like the pretend_object_file() interface,\n> > except that it's storing the objects on disk instead of in memory. Of\n> > course another way of doing that would be to stuff the object data into\n> > tempfiles and just put pointers into the in-memory cached_objects array.\n> >\n> > It's also not entirely foolproof (nor is the existing\n> > pretend_object_file()). Any operation which is fooled into thinking we\n> > have object X because it's in the fake-object list or the tmp_objdir may\n> > reference that object erroneously, creating a corruption. But it's still\n> > safer than allowing arbitrary writes into the tmp_objdir.\n> \n> Why is the pretend_object_file() interface safer? Does it disable the\n> textconv caching somehow? I don't see why it helps avoid this\n> problem.\n\nSo hopefully it's clearer now from what I wrote above, but just\nconnecting the dots:\n\n 1. Unrelated code calling write_object_file() would still write real,\n durable objects, as usual.\n\n 2. The write_object_file() \"skip this write\" optimization already\n ignores the pretend_object_file() objects while checking \"do we\n have this object\".\n\n> Interesting. I'm not familiar with the pretend_object_file() code,\n> but I think I catch what you're saying. I don't see anything in\n> object-file.c that allows removing items from the cache, as I would\n> want to do, but I'm sure I could add something.\n\nRight, it's pretty limited now, and there's only one caller. And in fact\nI've wanted to get rid of it, exactly because it can create the danger\nwe're discussing here. But I still think it's _less_ dangerous than\nfully replacing the object directory.\n\n> (I'm still not sure how this avoids the problem of things like\n> textconv caching trying to write an object that references one of\n> these, though. Should we just manually disable textconv caching\n> during a remerge-diff?)\n\nIt can't avoid it completely, but in general, the textconv cache should\nbe writing and referencing its own objects. Even if they happen to be\nthe same as something remerge-diff generated, it won't know that until\nit has tried to write it (and so that's why having write_object_file()\ncontinue to really write the object is important).\n\n\nHopefully that clears up my line of thinking. I do think a lot of this\nis kind of theoretical, in the sense that:\n\n - textconv caching is an obscure feature that we _could_ just disable\n during remerge-diffs\n\n - there's a reasonable chance that there isn't any other code that\n wants to write during a diff\n\nBut this is all scary and error-prone enough that I'd prefer an approach\nthat has the \"least surprise\". So any solution where random code calling\nwrite_object_file() _can't_ accidentally write to a throw-away directory\nseems like a safer less surprising thing.\n\nIt does mean that the remerge-diff code needs to be explicit in its\nobject writes to say \"this needs to go to the temporary area\" (whether\nit's a flag, or calling into a totally different function or subsystem).\nI'm hoping that's not hard to do (because its writes are done explicitly\nby the remerge-diff code), but I worry that it may be (because you are\nrelying on more generic tree code to the write under the hood).\n\n-Peff\n"},{"id":"437504","messageId":"YVVoXJo3DlPQd1A3@coredump.intra.peff.net","threadId":"56411","inReplyTo":"xmqqsfxof2hr.fsf@gitster.g","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-30T07:33:48Z","receivedAt":"2021-09-30T07:33:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Side note: The pretend_object_file() approach is actually even better,\n> > because we know the object is fake. So it does not confuse\n> > write_object_file()'s \"do we already have this object\" freshening\n> > check.\n> >\n> > I suspect it could even be made faster than the tmp_objdir approach.\n> > From our perspective, these objects really are tempfiles. So we could\n> > write them as such, not worrying about things like fsyncing them,\n> > naming them into place, etc. We could just write them out, then mmap\n> > the results, and put the pointers into cached_objects (currently it\n> > insists on malloc-ing a copy of the input buffer, but that seems like\n> > an easy extension to add).\n> >\n> > In fact, I think you could get away with just _one_ tempfile per\n> > merge. Open up one tempfile. Write out all of the objects you want to\n> > \"store\" into it in sequence, and record the lseek() offsets before and\n> > after for each object. Then mmap the whole result, and stuff the\n> > appropriate pointers (based on arithmetic with the offsets) into the\n> > cached_objects list.\n> \n> Cute. The remerge diff code path creates a full tree that records\n> the mechanical merge result. By hooking into the lowest layer of\n> write_object() interface, we'd serialize all objects in such a tree\n> in the order they are computed (bottom up from the leaf level, I'd\n> presume) into a single flat file ;-)\n\nI do still like this approach, but just two possible gotchas I was\nthinking of:\n\n - This side-steps all of our usual code for getting object data into\n memory. In general, I'd expect this content to not be too enormous,\n but it _could_ be if there are many / large blobs in the result. So\n we may end up with large maps. Probably not a big deal on modern\n 64-bit systems. Maybe an issue on 32-bit systems, just because of\n virtual address space.\n\n Likewise, we do support systems with NO_MMAP. They'd work here, but\n it would probably mean putting all that object data into the heap. I\n could live with that, given how rare such systems are these days, and\n that it only matters if you're using --remerge-diff with big blobs.\n\n - I wonder to what degree --remerge-diff benefits from omitting writes\n for objects we already have. I.e., if you are writing out a whole\n tree representing the conflicted state, then you don't want to write\n all of the trees that aren't interesting. Hopefully the code is\n already figuring out which paths the merge even touched, and ignoring\n the rest. It probably benefits performance-wise from\n write_object_file() deciding to skip some object writes, as well\n (e.g., for resolutions which the final tree already took, as they'd\n be in the merge commit). The whole pretend-we-have-this-object thing\n may want to likewise make sure we don't write out objects that we\n already have in the real odb.\n\n-Peff\n"},{"id":"437505","messageId":"YVVqLo6yNzPtXVv6@coredump.intra.peff.net","threadId":"56411","inReplyTo":"CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-30T07:41:34Z","receivedAt":"2021-09-30T07:41:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 09:22:32PM -0700, Elijah Newren wrote:\n\n> > Not your fault & this pre-dates your patch, but FWIW I prefer our APIs\n> > that don't have these \"hidden struct\" shenanigans (like say \"struct\n> > string_list\") so a caller could just access this, we can just declare it\n> > \"const\" appropriately.\n> >\n> > We're also all in-tree friends here, so having various accessors for no\n> > reason other than to access struct members seems a bit too much.\n> \n> That's a fair point, but just to play counterpoint for a minute...\n> \n> FWIW, I dislike when our public facing APIs are polluted with all\n> kinds of internal details. merge-recursive being a case in point.\n> When writing merge-ort, although I had a struct with public fields,\n> that struct also contained an opaque struct (pointer) within it to\n> hide several private fields. (I would have liked to hide or remove a\n> few more fields, but couldn't do so while the merge_recursive_options\n> struct was shared between both merge-ort and merge-recursive.)\n> \n> That said, I agree it can certainly be overdone, and tmp_objdir is\n> pretty simple. However, sometimes even in simple cases I like when\n> folks make use of an opaque struct because it means folks put some\n> effort into thinking more about the API that should be exposed.\n> That's something we as a project have often overlooked in the past, as\n> evidenced both by our one-shot usage mentality, and the existence of\n> external projects like libgit2 attempting to correct this design\n> shortcoming. I'd like git to move more towards being structured as a\n> reusable library as well as a useful command-line tool.\n\nRight, it was definitely a conscious decision to keep the tmp-objdir API\nas slim as possible, just because it's such a complicated and confusing\nthing in the first place.\n\nFor something like a strbuf, giving direct access to the fields makes\nsense. Exposing the details of how the struct works (like accessing\n\".buf\" as a NUL-terminated string) are part of its usefulness.\n\nBut tmp_objdir represents a more abstract concept, and I wanted to\ninsulate callers from the details.\n\nThat said, the notion of \"this is the path of the objdir\" is not that\ncontentious, so I don't mind it too much (but it would be a jump to\nexposing the details at all).\n\n-Peff\n"},{"id":"437506","messageId":"YVVrY0/kXfWHCWJ1@coredump.intra.peff.net","threadId":"56411","inReplyTo":"YVVmssXlaFM6yD5W@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-30T07:46:43Z","receivedAt":"2021-09-30T07:46:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2021 at 03:26:42AM -0400, Jeff King wrote:\n\n> > > If you remove the tmp_objdir as the primary as soon as you're done with\n> > > the merge, but before you run the diff, you might be OK, though.\n> > \n> > It has to be after I run the diff, because the diff needs access to\n> > the temporary files to diff against them.\n> \n> Right, of course. I was too fixated on the object-write part, forgetting\n> that the whole point of the exercise is to later read them back. :)\n\nAh, no, I remember what I was trying to say here. The distinction is\nbetween \"remove the tmp_objdir\" and \"remove it as the primary\".\n\nI.e., if you do this:\n\n 1. create tmp_objdir\n\n 2. make tmp_objdir primary for writes\n\n 3. run the \"merge\" half of remerge-diff, writing objects into the\n temporary space\n\n 4. stop having tmp_objdir as the primary; instead make it an alternate\n\n 5. run the diff\n\n 6. remove tmp_objdir totally\n\nThen step 5 can't accidentally write objects into the temporary space,\nbut it can still read them. So it's not entirely safe, but it's safer,\nand it would be a much smaller change.\n\nSome ways it could go wrong:\n\n - is it possible for the merge code to ever write an object? I kind of\n wonder if we'd ever do any cache-able transformations as part of a\n content-level merge. I don't think we do now, though.\n\n - in step 5, write_object_file() may still be confused by the presence\n of the to-be-thrown-away objects in the alternate. This is pretty\n unlikely, as it implies that the remerge-diff wrote a blob or tree\n that is byte-identical to something that the diff wants to write.\n\n-Peff\n"},{"id":"437507","messageId":"YVVuP8ReqaPi/Z5E@coredump.intra.peff.net","threadId":"56411","inReplyTo":"CABPp-BEioOUJRd6FSsmsDtYHhUy7xhr4YabdEmVKzkduo4g9TQ@mail.gmail.com","subject":"Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-30T07:58:55Z","receivedAt":"2021-09-30T07:58:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 28, 2021 at 11:25:20PM -0700, Elijah Newren wrote:\n\n> > Just brainstorming some alternatives:\n> >\n> > - we have diff.orderFile, etc. Could we stuff this data into a less\n> > confusing name (even just \"$filename.conflict_msg\"), and then provide\n> > a custom ordering to the diff code? I think it could be done by\n> > generating a static ordering ahead of time, but it might even just be\n> > possible to tell diffcore_order() to take the \".conflict_msg\"\n> > extension into account in its comparison function.\n> \n> I can't just go on the \".conflict_msg\" extension. As you noted above,\n> this scheme is not sufficient for avoiding collisions. So I need to\n> append extra \"cruft\" to the name in the case of collisions -- meaning\n> we can't special case on just that extension.\n\nSure, but we can call it filename.conflict_msg.1, etc, and the sort code\ncan pattern-match. It can never be fully robust (if you really did have\na foo.conflict_msg, we'd sort it differently), but I think it's OK if\nthe worst-case is that pathological trees get ordered slightly\nsub-optimally).\n\nThat said, I think it could also make sense to annotate the conflict\nfiles by giving them an unusual set of mode bits. That would be easier\nto recognize (and while real trees _could_ have silly modes, we do\ncomplain about them in fsck, so it shouldn't happen in practice).\n\n> I also don't like how diff.orderFile provides a global ordering of the\n> files listed, rather than providing some scheme for relative\n> orderings. That'd either force me to precompute the diff to determine\n> all the files that were different so I can list _all_ of them, or put\n> up with the fact that the files with non-content conflicts will be\n> listed very first in the output, even if their name is\n> 'zee-last-file.c' -- surprising users at the output ordering.\n> \n> This also means that if the user had their own ordering defined, then\n> I'm overriding it and messing up their ordering, which might be\n> problematic.\n\nAgreed. I do think it may be too horrible unless you teach\ndiffcore_order() to actually understand your annotations in code. But\nthat wouldn't be too hard if it's done in the mode bits.\n\nBut I do think anything that avoids these pseudo-files is going to be a\nlot cleaner overall.\n\n> > - there can be other non-diff data between the individual segments. For\n> > example, \"patch\" will skip over non-diff lines. And certainly in Git\n> > we have our own custom headers. I'm wondering if we could attach\n> > these annotations to the diff-pair somehow, and then show something\n> > like:\n> >\n> > diff --git a/foo.c b/foo.c\n> > index 1234abcd..5678cdef 100644\n> > conflict modify/delete foo.c\n> \n> A couple things here...\n\nI think Junio already indicated that we can pretty much make this look\nlike whatever we want. As soon as any reader sees \"conflict\", it should\nhappily ignore the rest as something it doesn't know about. And my short\nexample here was just meant to be illustrative. I agree it probably\nneeds more details (and the whole CONFLICT line that usually goes to\nstderr is probably the best thing).\n\n-Peff\n"},{"id":"437509","messageId":"87a6juv5yb.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"YVVuP8ReqaPi/Z5E@coredump.intra.peff.net","subject":"Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-30T08:09:09Z","receivedAt":"2021-09-30T08:11:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 30 2021, Jeff King wrote:\n\n> [...]\n> That said, I think it could also make sense to annotate the conflict\n> files by giving them an unusual set of mode bits. That would be easier\n> to recognize (and while real trees _could_ have silly modes, we do\n> complain about them in fsck, so it shouldn't happen in practice).\n\nWe don't complain about them in fsck. I've been meaning to get to fixing\nit, but my fixes were contingent on the rather big \"tree walk mode bits\"\nseries: https://lore.kernel.org/git/87wnu0r8tq.fsf@evledraar.gmail.com/\n"},{"id":"437521","messageId":"87tui2tckn.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"YVVoXJo3DlPQd1A3@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-30T13:16:19Z","receivedAt":"2021-09-30T13:31:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 30 2021, Jeff King wrote:\n\n> On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > Side note: The pretend_object_file() approach is actually even better,\n>> > because we know the object is fake. So it does not confuse\n>> > write_object_file()'s \"do we already have this object\" freshening\n>> > check.\n>> >\n>> > I suspect it could even be made faster than the tmp_objdir approach.\n>> > From our perspective, these objects really are tempfiles. So we could\n>> > write them as such, not worrying about things like fsyncing them,\n>> > naming them into place, etc. We could just write them out, then mmap\n>> > the results, and put the pointers into cached_objects (currently it\n>> > insists on malloc-ing a copy of the input buffer, but that seems like\n>> > an easy extension to add).\n>> >\n>> > In fact, I think you could get away with just _one_ tempfile per\n>> > merge. Open up one tempfile. Write out all of the objects you want to\n>> > \"store\" into it in sequence, and record the lseek() offsets before and\n>> > after for each object. Then mmap the whole result, and stuff the\n>> > appropriate pointers (based on arithmetic with the offsets) into the\n>> > cached_objects list.\n>> \n>> Cute. The remerge diff code path creates a full tree that records\n>> the mechanical merge result. By hooking into the lowest layer of\n>> write_object() interface, we'd serialize all objects in such a tree\n>> in the order they are computed (bottom up from the leaf level, I'd\n>> presume) into a single flat file ;-)\n>\n> I do still like this approach, but just two possible gotchas I was\n> thinking of:\n>\n> - This side-steps all of our usual code for getting object data into\n> memory. In general, I'd expect this content to not be too enormous,\n> but it _could_ be if there are many / large blobs in the result. So\n> we may end up with large maps. Probably not a big deal on modern\n> 64-bit systems. Maybe an issue on 32-bit systems, just because of\n> virtual address space.\n>\n> Likewise, we do support systems with NO_MMAP. They'd work here, but\n> it would probably mean putting all that object data into the heap. I\n> could live with that, given how rare such systems are these days, and\n> that it only matters if you're using --remerge-diff with big blobs.\n>\n> - I wonder to what degree --remerge-diff benefits from omitting writes\n> for objects we already have. I.e., if you are writing out a whole\n> tree representing the conflicted state, then you don't want to write\n> all of the trees that aren't interesting. Hopefully the code is\n> already figuring out which paths the merge even touched, and ignoring\n> the rest. It probably benefits performance-wise from\n> write_object_file() deciding to skip some object writes, as well\n> (e.g., for resolutions which the final tree already took, as they'd\n> be in the merge commit). The whole pretend-we-have-this-object thing\n> may want to likewise make sure we don't write out objects that we\n> already have in the real odb.\n\nI haven't benchmarked since my core.checkCollisions RFC patch[1]\nresulted in the somewhat related loose object cache patch from you, and\nnot with something like the midx, but just a note that on some setups\njust writing things out is faster than exhaustively checking if we\nabsolutely need to write things out.\n\nI also wonder how much if anything writing out the one file v.s. lots of\nloose objects is worthwhile on systems where we could write out those\nloose objects on a ramdisk, which is commonly available on e.g. Linux\ndistros these days out of the box. If you care about performance but not\nabout your transitory data using a ramdisk is generally much better than\nany other potential I/O optimization.\n\nFinally, and I don't mean to throw a monkey wrench into this whole\ndiscussion, so take this as a random musing: I wonder how much faster\nthis thing could be on its second run if instead of avoiding writing to\nthe store & cleaning up, it just wrote to the store, and then wrote\nanother object keyed on the git version and any revision paramaters\netc., and then pretty much just had to do a \"git cat-file -p \"\nto present the result to the user :)\n\nI suppose that would be throwing a lot more work at an eventual \"git gc\"\nthan we ever do now, so maybe it's a bit crazy, but I think it might be\nan interesting direction in general to (ab)use either the primary or\nsome secondary store in the .git dir as a semi-permanent cache of\nresolved queries from the likes of \"git log\".\n\n1. https://lore.kernel.org/git/20181028225023.26427-5-avarab@gmail.com/\n"},{"id":"437544","messageId":"87h7e2t9fh.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"CABPp-BG_qigBoirMGR-Yk9Niyxt0UmYCEqojsYxbSEarLAmraA@mail.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-30T14:17:12Z","receivedAt":"2021-09-30T14:41:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 28 2021, Elijah Newren wrote:\n\n> On Tue, Sep 28, 2021 at 1:00 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n>>\n>> I commented just now on how this API is duplicated between here &\n>> another in-flight series in\n>> https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/; Just\n>> comments on this patch here:\n>>\n>> > diff --git a/tmp-objdir.c b/tmp-objdir.c\n>> > index b8d880e3626..9f75a75d1c0 100644\n>> > --- a/tmp-objdir.c\n>> > +++ b/tmp-objdir.c\n>> > @@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)\n>> > return t->env.v;\n>> > }\n>> >\n>> > +const char *tmp_objdir_path(struct tmp_objdir *t)\n>> > +{\n>> > + return t->path.buf;\n>> > +}\n>>\n>> Not your fault & this pre-dates your patch, but FWIW I prefer our APIs\n>> that don't have these \"hidden struct\" shenanigans (like say \"struct\n>> string_list\") so a caller could just access this, we can just declare it\n>> \"const\" appropriately.\n>>\n>> We're also all in-tree friends here, so having various accessors for no\n>> reason other than to access struct members seems a bit too much.\n>\n> That's a fair point, but just to play counterpoint for a minute...\n>\n> FWIW, I dislike when our public facing APIs are polluted with all\n> kinds of internal details. merge-recursive being a case in point.\n> When writing merge-ort, although I had a struct with public fields,\n> that struct also contained an opaque struct (pointer) within it to\n> hide several private fields. (I would have liked to hide or remove a\n> few more fields, but couldn't do so while the merge_recursive_options\n> struct was shared between both merge-ort and merge-recursive.)\n>\n> That said, I agree it can certainly be overdone, and tmp_objdir is\n> pretty simple. However, sometimes even in simple cases I like when\n> folks make use of an opaque struct because it means folks put some\n> effort into thinking more about the API that should be exposed.\n> That's something we as a project have often overlooked in the past, as\n> evidenced both by our one-shot usage mentality, and the existence of\n> external projects like libgit2 attempting to correct this design\n> shortcoming. I'd like git to move more towards being structured as a\n> reusable library as well as a useful command-line tool.\n\n[This is somewhat of a continuation of\nhttps://lore.kernel.org/git/87lf3etaih.fsf@evledraar.gmail.com/].\n\nI like our APIs being usable, but right now we're not promising any\nmaintenance of APIs that work the same as git v2.30.0 or whatever.\n\nI think some external users directly link to libgit.a, but they're\ntracking whatever churn we have from release to release, just like\nsomeone maintaining out-of-tree linux kernel drivers.\n\nFor maintenance of git.git itself I think it's going to stay like that\nfor any foreseeable future[1].\n\nAnd that's before we even get into the practicalities of git's license\nlikely not winning over many (if any) libgit2 users. I think those users\nare if anything migrating from libgit2 to git's plumbing CLI, or in some\ncases folding what they needed into git.git itself (or just staying with\nlibgit2).\n\nSo assuming all of that I really don't see the point in general of\nhaving a \"foo\" field that has the equivalent of get_foo() and set_foo()\naccessors in the context of git.git.\n\nThat's something that's really useful if you're maintaining API and ABI\ncompatibility, but for us it's usually just verbosity. All the users are\nin-tree, so just add \"const\" as appropriate, or perhaps we should give\nthe field a \"priv\" name or whatever.\n\nObviously not everything should be done that way, e.g. being able to\nhave trace2 start/end points for something you'd otherwise push/clear\nfrom a \"struct string_list\" or whatever directly *is* usable, so it's\nnot black and white.\n\nI'm just saying thaht using convoluted patterns *just* to get in the way\nof some hypothetical code that's going to mess with your internals can\nmake things harder in other areas. E.g. the init/release pattern via\nmacros I mentioned in the linked E-Mail above.\n\n> [...]\n>> In this particular case I hadn't understood on a previous reading of\n>> tmp-objdir.c why this \"path\" isn't a statically allocated \"char\n>> path[PATH_MAX]\", or a least we that hardcoded \"1024\" should be a\n>> PATH_MAX, as it is in some other cases.\n>\n> Actually, PATH_MAX shouldn't be used:\n>\n> https://lore.kernel.org/git/7d1237c7-5a83-d766-7d93-5f0d59166067@web.de/\n> https://lore.kernel.org/git/20180720180427.GE22486@sigill.intra.peff.net/\n\nThanks. I didn't know about that. I've fixed (some of that) up in some\nsemi-related WIP work I've got.\n\n1. Aside: I *do* think it would be really useful for us to expose a\n stable C API eventually, using some of the same license\n workarounds/shenanigans (depending on your view) as the Linux kernel\n did. I.e. to say that \"we're GPLv2, but this small API is the\n equivalent of our syscall interface\".\n\n Rather than that API being something that would look anything like\n internal git.git users it would basically be a run_command() + Perl's\n unpack/pack + output formatter on steroids.\n\n I.e. we'd be able to invoke say a 'git cat-file --batch' in the same\n process as the caller by essentially calling cmd_cat_file()\n directly. Just with some callback replacements for \"provide this on\n stdin\" and \"this line accepts stdout\" without actually involving\n different processes or I/O.\n\n You'd then have the equivalent of a --format output that would just\n so happen to be in your OS's / C ABI's understood struct\n format. Similar to \"perldoc -f pack\".\n\n The maintenance burden of such an API would be trivial. It would\n basically be a handful of functions just to setup input/output\n callbacks.\n\n Most of the interface would be a rather convoluted construction of\n command-line parameters (like our own run_command() use), not ideal,\n but most users are after structured data and eliminating process\n overhead, and they'd get that.\n\n We would need to replace fprintf() and the like with some structured\n formatting mechanism, but that would be useful for non-API users as\n new commands learning a shiny new --format parameter. You could even\n use such a --format to have a non-C-API user get the same struct\n output, perhaps for direct mapping into memory, say if you didn't\n trust the built-in/command in question to not leak memory yet.\n\n I don't know if this has been suggested before, just my 0.02. It's\n also partially why I'm interested in making even-built-ins not leak\n memory with some of my parallel SANITIZE=leak work.\n\n Once you've got that you can usually call the cmd_whatever() in a\n loop without much effort, and once you've got that, well, see above\n :)\n\n \n"},{"id":"437555","messageId":"87a6jut36p.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"ce95b82fc492d48fa6022df424f9a303a1c70ad4.1630376800.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/7] merge-ort: capture and print ll-merge warnings in our preferred fashion","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-30T16:53:45Z","receivedAt":"2021-09-30T16:53:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n\n> @@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '\n> \tprintf \"\\0\\0\" >binary-file &&\n> \tgit add binary-file &&\n> \tgit commit -m binary2 &&\n> -\ttest_must_fail git merge F >merge.out 2>merge.err &&\n> -\tgrep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.err\n> +\tif test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n> +\tthen\n> +\t\ttest_must_fail git merge F >merge.out &&\n> +\t\tgrep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.out\n> +\telse\n> +\t\ttest_must_fail git merge F >merge.out 2>merge.err &&\n> +\t\tgrep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.err\n> +\tfi\n> '\n\nTo save readers from eyeballing if a single character has changed here,\nwhich I don't think it has, just do:\n\nif ...\nthen\n\tcmd >actual\nelse\n\tother cmd >actual\nfi &&\ngrep [...]\n\nI.e. no need to duplicate the \"grep\" here just because of merge.out v.s. merge.err.\n\n[...]\n\n> -\ttest_must_fail git merge bin-main 2>stderr &&\n> -\tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stderr\n> +\tif test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n> +\tthen\n> +\t\ttest_must_fail git merge bin-main >stdout &&\n> +\t\tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stdout\n> +\telse\n> +\t\ttest_must_fail git merge bin-main 2>stderr &&\n> +\t\tgrep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stderr\n> +\tfi\n\n\nditto.\n"},{"id":"437571","messageId":"20210930192537.GA4151@neerajsi-x1.localdomain","threadId":"56411","inReplyTo":"YVVmssXlaFM6yD5W@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Neeraj Singh","fromEmail":"nksingh85@gmail.com","sentAt":"2021-09-30T19:25:37Z","receivedAt":"2021-09-30T19:25:43Z","isPatch":true,"sender":{"key":"nksingh85@gmail.com","avatar":null},"body":"On Thu, Sep 30, 2021 at 03:26:42AM -0400, Jeff King wrote:\n> On Tue, Sep 28, 2021 at 10:05:31PM -0700, Elijah Newren wrote:\n> \n> > So there are other parts of running `git log` that are not read-only,\n> > besides what I'm doing. That's unfortunate. Can we just disable\n> > those things in this section of the code?\n> \n> It might be possible for the cache (we'd still generate entries, but\n> just not write them). It's \"just\" an optimization, though it's possible\n> there are cases where it's really annoying to do so. I'd imagine those\n> are a stretch. I do use textconv to decrypt gpg blobs with a key on a\n> hardware token, for example. I don't require a touch for each one,\n> though, nor would I use textconv caching anyway (since the point is not\n> to store the unencrypted contents).\n> \n> It wouldn't necessarily be OK to disable object writes in general.\n> Though I'd think since a diff is _conceptually_ a read-only operation\n> that any writes it wants to do would be in the same caching /\n> optimization boat. I'd also worry about the maintenance cost of having\n> to annotate any such case.\n> \n> > Also, I think it makes sense that textconv caching causes a problem\n> > via writing new blobs and trees *and* refs that reference these blobs\n> > and trees. However, I'm not sure I'm really grasping it, because I\n> > don't see how your solutions are safe either. I'll mention more after\n> > each one.\n> \n> Heh. So I originally wrote more on this in my earlier email, but worried\n> I was getting too into the weeds and would confuse you, so I pared it\n> down. But I managed to be confusing anyway. :)\n> \n> Here's one mental model. If you have a temporary object store, then\n> these are the bad things that can happen:\n> \n> 1. Some code may write an object to it, thinking the data is saved,\n> but we later throw it away.\n> \n> 2. Some code may _skip_ writing an object, thinking that we already\n> have that object available (when in fact we'll later throw it\n> away).\n> \n> 3. Some code may record the fact that we have an object in memory, and\n> then later (after the temporary object store is not in use), may\n> point to it when writing an object or reference.\n> \n> Pointing the primary object-dir at a temporary directory (like your\n> patch does) means we're subject to all three.\n> \n> Problem (1) happens because unrelated code may not realize we've swapped\n> out the primary object writing code for this throw-away directory. But\n> we could require writers to explicitly indicate that they want to write\n> to the temporary area, while allowing other writes to go to the regular\n> object store. This could be done with a flag or variant of\n> write_object_file(). Or if the temporary area isn't a regular object\n> directory at all (like the whole tempfile / pretend thing), then this\n> happens automatically, because the regular object-write paths don't even\n> know about our temporary area.\n> \n> Problem (2) is mostly handled inside write_object_file(). It will\n> optimize out writes of objects we already have. So it needs to know when\n> we have an object \"for real\", and when we just have it in our temporary\n> store. If we make the tmp_objdir the primary, then it doesn't know this\n> (though we could teach it to check. In the pretend_object_file() system,\n> it already knows this (it uses the \"freshen\" code paths which actually\n> want to find the entry in the filesystem).\n> \n> Problem (3) is the hardest one, because we don't really distinguish\n> between readers and writers. So if I call has_object_file(), is it\n> because I want to access the object? Or is it because I want to generate\n> a new reference to it, which must ensure that it exists? One of those is\n> OK to look at the tmp_objdir (and indeed, is the whole point of making\n> it in the first place), and the other is the first step to corrupting\n> the repository. ;)\n> \n\nThis is a good taxonomy. I think these problems apply to any transactional\nsystem. If you keep state outside of the transaction, then you may wind up\nwith inconsistency. Problem (3) is the worst and kind of subsumes (2). If\nwe think an object is durable with a specific oid, we probably are going to\nwrite that oid out somewhere else that is outside of the transaction. If\nwe write it to the ODB (e.g. in a tree or some blob) without first caching\nit in memory, the references in the ODB will be equally durable as the referees.\n\n\n> With the environment variables we set up for running external commands\n> (like hooks) in the receive-pack quarantine, one bit of safety we have\n> is to prevent ref writes entirely in those commands. That works\n> reasonably well. Even if they may write new objects that might get\n> thrown away (if the push is rejected), they'd ultimately need to point\n> to those objects with a ref. And because those hooks spend their\n> _entire_ run-time in the quarantine state, they can't ever write a ref.\n> \n> Whereas doing it in-process is a little dicier. Right now the textconv\n> cache writes out a new tree for every entry we add, so it happens to do\n> the ref write while the tmp-objdir is still in place. But as this\n> comment notes:\n> \n> $ git grep -B6 notes_cache_write diff.c\n> diff.c- /*\n> diff.c- * we could save up changes and flush them all at the end,\n> diff.c- * but we would need an extra call after all diffing is done.\n> diff.c- * Since generating a cache entry is the slow path anyway,\n> diff.c- * this extra overhead probably isn't a big deal.\n> diff.c- */\n> diff.c: notes_cache_write(textconv->cache);\n> \n> this is slow, and it would be perfectly reasonable to flush the cache at\n> the end of the process (e.g., using an atexit() handler). In which case\n> we'd hit this problem (3) exactly: we'd generate and remember objects\n> during the tmp-objdir period, but only later actually reference them.\n> This is more likely than the receive-pack hook case, because we're doing\n> it all in process.\n> \n\nIs there state we care about besides objects and refs? Maybe we need to\nintroduce a transactional system for the entire repository like we have\nfor the refs-db. In-process code that cares about oids that might be\nonly transactionally present can register a callback for when the transaction\ncommits or rollsback. I'm not volunteering to write this, btw :).\n\nRight now, disabling Ref updates is a pretty good compromise, since that's the\none definite piece of durable state outside the ODB that can point in. In memory\nstate is generally hard to solve and could wind up persisting in either the ODB\nor the Refs or both.\n\n> > > If you remove the tmp_objdir as the primary as soon as you're done with\n> > > the merge, but before you run the diff, you might be OK, though.\n> > \n> > It has to be after I run the diff, because the diff needs access to\n> > the temporary files to diff against them.\n> \n> Right, of course. I was too fixated on the object-write part, forgetting\n> that the whole point of the exercise is to later read them back. :)\n> \n> > > If not, then I think the solution is probably not to install this as the\n> > > \"primary\", but rather:\n> > >\n> > > - do the specific remerge-diff writes we want using a special \"write\n> > > to this object dir\" variant of write_object_file()\n> > >\n> > > - install the tmp_objdir as an alternate, so the reading side (which\n> > > is diff code that doesn't otherwise know about our remerge-diff\n> > > trickery) can find them\n> > >\n> > > And that lets other writers avoid writing into the temporary area\n> > > accidentally.\n> > \n> > Doesn't this have the same problem? If something similar to textconv\n> > caching were to create new trees that referenced the existing blobs\n> > and then added a ref that referred to that tree, then you have the\n> > exact same problem, right? The new tree and the new ref would be\n> > corrupt as soon as the tmp-objdir went away. Whether or not other\n> > callers write to the temporary area isn't the issue, it's whether they\n> > write refs that can refer to anything from the temporary area. Or am\n> > I missing something?\n> \n> The key thing here is in the first step, where remerge-diff is\n> explicitly saying \"I want to write to this temporary object-dir\". But\n> the textconv-cache code does not; it gets to write to the normal spot.\n> So it avoids problem (1).\n> \n> You're right that it does not avoid problem (3) exactly. But triggering\n> that would require some code not just writing other objects or\n> references while the tmp-objdir is in place, but specifically\n> referencing the objects that remerge-diff did put into the tmp-objdir.\n> That seems a lot less likely to me (because the thing we're most worried\n> about is unrelated code that just happens to write while the tmp-objdir\n> is in place).\n> \n> > > In that sense this is kind of like the pretend_object_file() interface,\n> > > except that it's storing the objects on disk instead of in memory. Of\n> > > course another way of doing that would be to stuff the object data into\n> > > tempfiles and just put pointers into the in-memory cached_objects array.\n> > >\n> > > It's also not entirely foolproof (nor is the existing\n> > > pretend_object_file()). Any operation which is fooled into thinking we\n> > > have object X because it's in the fake-object list or the tmp_objdir may\n> > > reference that object erroneously, creating a corruption. But it's still\n> > > safer than allowing arbitrary writes into the tmp_objdir.\n> > \n> > Why is the pretend_object_file() interface safer? Does it disable the\n> > textconv caching somehow? I don't see why it helps avoid this\n> > problem.\n> \n> So hopefully it's clearer now from what I wrote above, but just\n> connecting the dots:\n> \n> 1. Unrelated code calling write_object_file() would still write real,\n> durable objects, as usual.\n> \n> 2. The write_object_file() \"skip this write\" optimization already\n> ignores the pretend_object_file() objects while checking \"do we\n> have this object\".\n> \n> > Interesting. I'm not familiar with the pretend_object_file() code,\n> > but I think I catch what you're saying. I don't see anything in\n> > object-file.c that allows removing items from the cache, as I would\n> > want to do, but I'm sure I could add something.\n> \n> Right, it's pretty limited now, and there's only one caller. And in fact\n> I've wanted to get rid of it, exactly because it can create the danger\n> we're discussing here. But I still think it's _less_ dangerous than\n> fully replacing the object directory.\n> \n> > (I'm still not sure how this avoids the problem of things like\n> > textconv caching trying to write an object that references one of\n> > these, though. Should we just manually disable textconv caching\n> > during a remerge-diff?)\n> \n> It can't avoid it completely, but in general, the textconv cache should\n> be writing and referencing its own objects. Even if they happen to be\n> the same as something remerge-diff generated, it won't know that until\n> it has tried to write it (and so that's why having write_object_file()\n> continue to really write the object is important).\n> \n> \n> Hopefully that clears up my line of thinking. I do think a lot of this\n> is kind of theoretical, in the sense that:\n> \n> - textconv caching is an obscure feature that we _could_ just disable\n> during remerge-diffs\n> \n> - there's a reasonable chance that there isn't any other code that\n> wants to write during a diff\n> \n> But this is all scary and error-prone enough that I'd prefer an approach\n> that has the \"least surprise\". So any solution where random code calling\n> write_object_file() _can't_ accidentally write to a throw-away directory\n> seems like a safer less surprising thing.\n> \n\nThere is something nice to be said for having the cached state go away\nwith the ephemeral objects, since that caching refers to objects that\nmay never be seen again.\n\nAny solution which makes in-flight transactional state visible (either as\nthe primary writing spot or as a readable alternate) will suffer from state\nleakage for non-transactionally-aware readers/writers.\n\nThe bulk-fsync code has some advantage in that it is more constrained in what\nit's calling while the tmp objdir is active. Also, in typical cases, the\ntmp objdir will be committed rather than discarded. But I believe it suffers\nfrom the same essential issue.\n\nCan we press forward, and just try to make sure all the in-process stuff\nis aware of temporary object state? Maybe we can hook up the temp odb stuff\nwith the refsdb transactions into a single system.\n\nWe can make the tmp object directory stuff more prevalent by using it in\ngit-apply and the merge stategy, and by using bulk-checkin while writing out\nthe cached tree and commits. This should flush out any issues and keep other\nparts of the codebase honest.\n\n> It does mean that the remerge-diff code needs to be explicit in its\n> object writes to say \"this needs to go to the temporary area\" (whether\n> it's a flag, or calling into a totally different function or subsystem).\n> I'm hoping that's not hard to do (because its writes are done explicitly\n> by the remerge-diff code), but I worry that it may be (because you are\n> relying on more generic tree code to the write under the hood).\n> \n\nI don't think that really helps the trickiest problems. The textconv cache,\nfor instance, may now be writing notes that refer to nonexistent objects.\nThat's fine from a correctness point of view, but now we're polluting the repo\nwith state that won't be gc'ed and won't ever be accessed.\n\nIMO, it might be better for the textconv cache to have a single notes root\ntree (rather than a notes 'branch' with a commit history). We would want to\nput a ref to this tree somewhere inside the object directory itself so that\nit's transactionally consistent with the rest of the object state. For instance,\nwe could store the current textconv notes tree root in .git/objects/info/textconv-cache.\n\nThanks,\nNeeraj\n\n"},{"id":"437572","messageId":"xmqqfstlbzq9.fsf@gitster.g","threadId":"56411","inReplyTo":"YVVmssXlaFM6yD5W@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-30T20:00:30Z","receivedAt":"2021-09-30T20:00:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > If not, then I think the solution is probably not to install this as the\n>> > \"primary\", but rather:\n>> >\n>> > - do the specific remerge-diff writes we want using a special \"write\n>> > to this object dir\" variant of write_object_file()\n>> >\n>> > - install the tmp_objdir as an alternate, so the reading side (which\n>> > is diff code that doesn't otherwise know about our remerge-diff\n>> > trickery) can find them\n>> >\n>> > And that lets other writers avoid writing into the temporary area\n>> > accidentally.\n>> \n>> ...\n>\n> The key thing here is in the first step, where remerge-diff is\n> explicitly saying \"I want to write to this temporary object-dir\". But\n> the textconv-cache code does not; it gets to write to the normal spot.\n> So it avoids problem (1).\n>\n> You're right that it does not avoid problem (3) exactly. But triggering\n> that would require some code not just writing other objects or\n> references while the tmp-objdir is in place, but specifically\n> referencing the objects that remerge-diff did put into the tmp-objdir.\n> That seems a lot less likely to me (because the thing we're most worried\n> about is unrelated code that just happens to write while the tmp-objdir\n> is in place).\n\nI do not offhand remember if a new object creation codepath that\navoids writing an object that we already have considers an object we\nfind in an alternate as \"we have it\". If it does not and we make a\nduplicated copy in our primary object store, then it would be OK,\nbut otherwise, those that know tmp_objdir is an alternate may still\ntry to write a new object and find that the same object already\nexists in an alternate. Once tmp_objdir is gone, we would end up\nwith a corrupt repository, right?\n\nfreshen_loose_object() seems to go to check_and_freshen() which\nconsult check_and_freshen_nonlocal() before returning yes/no, so\nhaving the same object in an alternate does seem to count as having\none.\n\n> So hopefully it's clearer now from what I wrote above, but just\n> connecting the dots:\n>\n> 1. Unrelated code calling write_object_file() would still write real,\n> durable objects, as usual.\n>\n> 2. The write_object_file() \"skip this write\" optimization already\n> ignores the pretend_object_file() objects while checking \"do we\n> have this object\".\n\nYup for (2)---the pretend interface is safer than alternate in that\nsense.\n\nBut the pretend interface was designed to just hold a handful of\nphoney objects in core. I am not sure if it is a good fit for this\nuse case.\n\n> But this is all scary and error-prone enough that I'd prefer an approach\n> that has the \"least surprise\". So any solution where random code calling\n> write_object_file() _can't_ accidentally write to a throw-away directory\n> seems like a safer less surprising thing.\n>\n> It does mean that the remerge-diff code needs to be explicit in its\n> object writes to say \"this needs to go to the temporary area\" (whether\n> it's a flag, or calling into a totally different function or subsystem).\n\n... and also writes by others that happens to write the same object\nas what is already in the temporary area should be made to the real\nobject store. Then we would be much safer.\n\n> I'm hoping that's not hard to do (because its writes are done explicitly\n> by the remerge-diff code), but I worry that it may be (because you are\n> relying on more generic tree code to the write under the hood).\n\nThanks.\n"},{"id":"437573","messageId":"xmqqbl49bzfy.fsf@gitster.g","threadId":"56411","inReplyTo":"YVVrY0/kXfWHCWJ1@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-30T20:06:41Z","receivedAt":"2021-09-30T20:06:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> - is it possible for the merge code to ever write an object? I kind of\n> wonder if we'd ever do any cache-able transformations as part of a\n> content-level merge. I don't think we do now, though.\n\nHow is virtual merge base, result of an inner merge that recursively\nmerging two merge bases, fed to an outer merge as the ancestor?\nIsn't it written as a tree object by the inner merge as a tree with\nfull of blob objects, so the outer merge does not have to treat a\nmerge base tree that is virtual any specially from a sole merge\nbase?\n"},{"id":"437575","messageId":"xmqq5yuhbyud.fsf@gitster.g","threadId":"56411","inReplyTo":"20210930192537.GA4151@neerajsi-x1.localdomain","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-30T20:19:38Z","receivedAt":"2021-09-30T20:19:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Neeraj Singh writes:\n\n> Is there state we care about besides objects and refs? Maybe we need to\n> introduce a transactional system for the entire repository like we have\n> for the refs-db. In-process code that cares about oids that might be\n> only transactionally present can register a callback for when the transaction\n> commits or rollsback. I'm not volunteering to write this, btw :).\n\nI take it to mean by \"objects and refs\" that we care only about\nobjects that are reachable from the refs? The index (the primary\nindex array plus extensions that record object names) also has\nanchoring points we care about (ask \"git fsck\" what they are).\n\n> We can make the tmp object directory stuff more prevalent by using it in\n> git-apply and the merge stategy, and by using bulk-checkin while writing out\n> the cached tree and commits. This should flush out any issues and keep other\n> parts of the codebase honest.\n\nI wonder what's so wrong about cruft objects in the main object\nstore that may be gc-ed away laster, though. IOW, I wonder if it is\nsimpler to treat the quarantine while accepting dubious foreign data\nvia \"git push\" in receive-pack as an exception, not a norm.\n\n"},{"id":"437577","messageId":"YVYlZhl6uACu+UGG@coredump.intra.peff.net","threadId":"56411","inReplyTo":"87tui2tckn.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-09-30T21:00:22Z","receivedAt":"2021-09-30T21:00:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2021 at 03:16:19PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I also wonder how much if anything writing out the one file v.s. lots of\n> loose objects is worthwhile on systems where we could write out those\n> loose objects on a ramdisk, which is commonly available on e.g. Linux\n> distros these days out of the box. If you care about performance but not\n> about your transitory data using a ramdisk is generally much better than\n> any other potential I/O optimization.\n\nI'd think in general we won't be using a ramdisk, because tmp_objdir is\nputting its directory inside $GIT_DIR/objects. It doesn't _have_ to, but\nusing a ramdisk works against its original purpose (which was to store\npotentially quite a lot of data from an incoming push, and to be able to\nrename it cheaply into its final resting place).\n\nIt would probably not be too hard to provide a flag that indicates the\nintended use, though (and then we decide where to create the temporary\ndirectory based on that).\n\n> Finally, and I don't mean to throw a monkey wrench into this whole\n> discussion, so take this as a random musing: I wonder how much faster\n> this thing could be on its second run if instead of avoiding writing to\n> the store & cleaning up, it just wrote to the store, and then wrote\n> another object keyed on the git version and any revision paramaters\n> etc., and then pretty much just had to do a \"git cat-file -p \"\n> to present the result to the user :)\n> \n> I suppose that would be throwing a lot more work at an eventual \"git gc\"\n> than we ever do now, so maybe it's a bit crazy, but I think it might be\n> an interesting direction in general to (ab)use either the primary or\n> some secondary store in the .git dir as a semi-permanent cache of\n> resolved queries from the likes of \"git log\".\n\nI don't think it's crazy to just write the objects to the main object\nstore. We already generate cruft objects for some other operations\n(Junio asked elsewhere in the thread about virtual trees for recursive\nmerges; I don't know the answer offhand, but I'd guess we do there).\nThey do get cleaned up eventually.\n\nI'm not sure it helps performance much by itself. In a merge (or even\njust writing a tree out from the index), by the time you realize you\nalready have the object, you've done most of the work to generate it.\n\nI think what you're describing is to make some kind of cache structure\non top. That might be sensible (and indeed, the index already does this\nwith the cachetree extension). But it can also easily come later if the\nobjects are just in the regular odb.\n\n-Peff\n"},{"id":"437596","messageId":"CABPp-BFyh239+7FLuUuoRJJbcE8NxHZABPTXPmSQhvRSPCiGvQ@mail.gmail.com","threadId":"56411","inReplyTo":"87a6jut36p.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/7] merge-ort: capture and print ll-merge warnings in our preferred fashion","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T01:54:10Z","receivedAt":"2021-10-01T01:54:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 30, 2021 at 9:53 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n>\n> > @@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '\n> > printf \"\\0\\0\" >binary-file &&\n> > git add binary-file &&\n> > git commit -m binary2 &&\n> > - test_must_fail git merge F >merge.out 2>merge.err &&\n> > - grep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.err\n> > + if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n> > + then\n> > + test_must_fail git merge F >merge.out &&\n> > + grep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.out\n> > + else\n> > + test_must_fail git merge F >merge.out 2>merge.err &&\n> > + grep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.err\n> > + fi\n> > '\n>\n> To save readers from eyeballing if a single character has changed here,\n> which I don't think it has, just do:\n>\n> if ...\n> then\n> cmd >actual\n> else\n> other cmd >actual\n\nDo you mean\n other cmd >/dev/null 2>actual\n?\n\n\n> fi &&\n> grep [...]\n>\n> I.e. no need to duplicate the \"grep\" here just because of merge.out v.s. merge.err.\n>\n> [...]\n>\n> > - test_must_fail git merge bin-main 2>stderr &&\n> > - grep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stderr\n> > + if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n> > + then\n> > + test_must_fail git merge bin-main >stdout &&\n> > + grep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stdout\n> > + else\n> > + test_must_fail git merge bin-main 2>stderr &&\n> > + grep -i \"warning.*cannot merge.*HEAD vs. bin-main\" stderr\n> > + fi\n>\n>\n> ditto.\n"},{"id":"437598","messageId":"CABPp-BFimGLnP331aFqLx0x-uQG7wHwSoN25fcYrPzWU93fn6Q@mail.gmail.com","threadId":"56411","inReplyTo":"YVVuP8ReqaPi/Z5E@coredump.intra.peff.net","subject":"Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T02:07:21Z","receivedAt":"2021-10-01T02:08:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 30, 2021 at 12:58 AM Jeff King wrote:\n>\n> On Tue, Sep 28, 2021 at 11:25:20PM -0700, Elijah Newren wrote:\n>\n> > > Just brainstorming some alternatives:\n> > >\n> > > - we have diff.orderFile, etc. Could we stuff this data into a less\n> > > confusing name (even just \"$filename.conflict_msg\"), and then provide\n> > > a custom ordering to the diff code? I think it could be done by\n> > > generating a static ordering ahead of time, but it might even just be\n> > > possible to tell diffcore_order() to take the \".conflict_msg\"\n> > > extension into account in its comparison function.\n> >\n> > I can't just go on the \".conflict_msg\" extension. As you noted above,\n> > this scheme is not sufficient for avoiding collisions. So I need to\n> > append extra \"cruft\" to the name in the case of collisions -- meaning\n> > we can't special case on just that extension.\n>\n> Sure, but we can call it filename.conflict_msg.1, etc, and the sort code\n> can pattern-match. It can never be fully robust (if you really did have\n> a foo.conflict_msg, we'd sort it differently), but I think it's OK if\n> the worst-case is that pathological trees get ordered slightly\n> sub-optimally).\n>\n> That said, I think it could also make sense to annotate the conflict\n> files by giving them an unusual set of mode bits. That would be easier\n> to recognize (and while real trees _could_ have silly modes, we do\n> complain about them in fsck, so it shouldn't happen in practice).\n\nI tried giving things unusual mode bits in early versions of merge-ort\nfor other reasons. It doesn't work: canon_mode() will \"fix\" the\nunusualness so there's no way for the reader to know that they had\nunusual bits.\n\nBut, as you said later, avoiding these pseudo-files is going to be\ncleaner anyway, so let's just do that.\n\n\n\n> > I also don't like how diff.orderFile provides a global ordering of the\n> > files listed, rather than providing some scheme for relative\n> > orderings. That'd either force me to precompute the diff to determine\n> > all the files that were different so I can list _all_ of them, or put\n> > up with the fact that the files with non-content conflicts will be\n> > listed very first in the output, even if their name is\n> > 'zee-last-file.c' -- surprising users at the output ordering.\n> >\n> > This also means that if the user had their own ordering defined, then\n> > I'm overriding it and messing up their ordering, which might be\n> > problematic.\n>\n> Agreed. I do think it may be too horrible unless you teach\n> diffcore_order() to actually understand your annotations in code. But\n> that wouldn't be too hard if it's done in the mode bits.\n>\n> But I do think anything that avoids these pseudo-files is going to be a\n> lot cleaner overall.\n>\n> > > - there can be other non-diff data between the individual segments. For\n> > > example, \"patch\" will skip over non-diff lines. And certainly in Git\n> > > we have our own custom headers. I'm wondering if we could attach\n> > > these annotations to the diff-pair somehow, and then show something\n> > > like:\n> > >\n> > > diff --git a/foo.c b/foo.c\n> > > index 1234abcd..5678cdef 100644\n> > > conflict modify/delete foo.c\n> >\n> > A couple things here...\n>\n> I think Junio already indicated that we can pretty much make this look\n> like whatever we want. As soon as any reader sees \"conflict\", it should\n> happily ignore the rest as something it doesn't know about. And my short\n> example here was just meant to be illustrative. I agree it probably\n> needs more details (and the whole CONFLICT line that usually goes to\n> stderr is probably the best thing).\n>\n> -Peff\n"},{"id":"437600","messageId":"CABPp-BGqxEoMRB4RDty+QzdwXK=1eKdEbQ7UoMPAmwqVsXCz8Q@mail.gmail.com","threadId":"56411","inReplyTo":"YVVmssXlaFM6yD5W@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T02:23:14Z","receivedAt":"2021-10-01T02:23:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 30, 2021 at 12:26 AM Jeff King wrote:\n>\n> On Tue, Sep 28, 2021 at 10:05:31PM -0700, Elijah Newren wrote:\n>\n> > So there are other parts of running `git log` that are not read-only,\n> > besides what I'm doing. That's unfortunate. Can we just disable\n> > those things in this section of the code?\n>\n> It might be possible for the cache (we'd still generate entries, but\n> just not write them). It's \"just\" an optimization, though it's possible\n> there are cases where it's really annoying to do so. I'd imagine those\n> are a stretch. I do use textconv to decrypt gpg blobs with a key on a\n> hardware token, for example. I don't require a touch for each one,\n> though, nor would I use textconv caching anyway (since the point is not\n> to store the unencrypted contents).\n>\n> It wouldn't necessarily be OK to disable object writes in general.\n> Though I'd think since a diff is _conceptually_ a read-only operation\n> that any writes it wants to do would be in the same caching /\n> optimization boat. I'd also worry about the maintenance cost of having\n> to annotate any such case.\n>\n> > Also, I think it makes sense that textconv caching causes a problem\n> > via writing new blobs and trees *and* refs that reference these blobs\n> > and trees. However, I'm not sure I'm really grasping it, because I\n> > don't see how your solutions are safe either. I'll mention more after\n> > each one.\n>\n> Heh. So I originally wrote more on this in my earlier email, but worried\n> I was getting too into the weeds and would confuse you, so I pared it\n> down. But I managed to be confusing anyway. :)\n>\n> Here's one mental model. If you have a temporary object store, then\n> these are the bad things that can happen:\n>\n> 1. Some code may write an object to it, thinking the data is saved,\n> but we later throw it away.\n>\n> 2. Some code may _skip_ writing an object, thinking that we already\n> have that object available (when in fact we'll later throw it\n> away).\n>\n> 3. Some code may record the fact that we have an object in memory, and\n> then later (after the temporary object store is not in use), may\n> point to it when writing an object or reference.\n>\n> Pointing the primary object-dir at a temporary directory (like your\n> patch does) means we're subject to all three.\n>\n> Problem (1) happens because unrelated code may not realize we've swapped\n> out the primary object writing code for this throw-away directory. But\n> we could require writers to explicitly indicate that they want to write\n> to the temporary area, while allowing other writes to go to the regular\n> object store. This could be done with a flag or variant of\n> write_object_file(). Or if the temporary area isn't a regular object\n> directory at all (like the whole tempfile / pretend thing), then this\n> happens automatically, because the regular object-write paths don't even\n> know about our temporary area.\n>\n> Problem (2) is mostly handled inside write_object_file(). It will\n> optimize out writes of objects we already have. So it needs to know when\n> we have an object \"for real\", and when we just have it in our temporary\n> store. If we make the tmp_objdir the primary, then it doesn't know this\n> (though we could teach it to check. In the pretend_object_file() system,\n> it already knows this (it uses the \"freshen\" code paths which actually\n> want to find the entry in the filesystem).\n>\n> Problem (3) is the hardest one, because we don't really distinguish\n> between readers and writers. So if I call has_object_file(), is it\n> because I want to access the object? Or is it because I want to generate\n> a new reference to it, which must ensure that it exists? One of those is\n> OK to look at the tmp_objdir (and indeed, is the whole point of making\n> it in the first place), and the other is the first step to corrupting\n> the repository. ;)\n>\n> With the environment variables we set up for running external commands\n> (like hooks) in the receive-pack quarantine, one bit of safety we have\n> is to prevent ref writes entirely in those commands. That works\n> reasonably well. Even if they may write new objects that might get\n> thrown away (if the push is rejected), they'd ultimately need to point\n> to those objects with a ref. And because those hooks spend their\n> _entire_ run-time in the quarantine state, they can't ever write a ref.\n>\n> Whereas doing it in-process is a little dicier. Right now the textconv\n> cache writes out a new tree for every entry we add, so it happens to do\n> the ref write while the tmp-objdir is still in place. But as this\n> comment notes:\n>\n> $ git grep -B6 notes_cache_write diff.c\n> diff.c- /*\n> diff.c- * we could save up changes and flush them all at the end,\n> diff.c- * but we would need an extra call after all diffing is done.\n> diff.c- * Since generating a cache entry is the slow path anyway,\n> diff.c- * this extra overhead probably isn't a big deal.\n> diff.c- */\n> diff.c: notes_cache_write(textconv->cache);\n>\n> this is slow, and it would be perfectly reasonable to flush the cache at\n> the end of the process (e.g., using an atexit() handler). In which case\n> we'd hit this problem (3) exactly: we'd generate and remember objects\n> during the tmp-objdir period, but only later actually reference them.\n> This is more likely than the receive-pack hook case, because we're doing\n> it all in process.\n>\n> > > If you remove the tmp_objdir as the primary as soon as you're done with\n> > > the merge, but before you run the diff, you might be OK, though.\n> >\n> > It has to be after I run the diff, because the diff needs access to\n> > the temporary files to diff against them.\n>\n> Right, of course. I was too fixated on the object-write part, forgetting\n> that the whole point of the exercise is to later read them back. :)\n>\n> > > If not, then I think the solution is probably not to install this as the\n> > > \"primary\", but rather:\n> > >\n> > > - do the specific remerge-diff writes we want using a special \"write\n> > > to this object dir\" variant of write_object_file()\n> > >\n> > > - install the tmp_objdir as an alternate, so the reading side (which\n> > > is diff code that doesn't otherwise know about our remerge-diff\n> > > trickery) can find them\n> > >\n> > > And that lets other writers avoid writing into the temporary area\n> > > accidentally.\n> >\n> > Doesn't this have the same problem? If something similar to textconv\n> > caching were to create new trees that referenced the existing blobs\n> > and then added a ref that referred to that tree, then you have the\n> > exact same problem, right? The new tree and the new ref would be\n> > corrupt as soon as the tmp-objdir went away. Whether or not other\n> > callers write to the temporary area isn't the issue, it's whether they\n> > write refs that can refer to anything from the temporary area. Or am\n> > I missing something?\n>\n> The key thing here is in the first step, where remerge-diff is\n> explicitly saying \"I want to write to this temporary object-dir\". But\n> the textconv-cache code does not; it gets to write to the normal spot.\n> So it avoids problem (1).\n>\n> You're right that it does not avoid problem (3) exactly. But triggering\n> that would require some code not just writing other objects or\n> references while the tmp-objdir is in place, but specifically\n> referencing the objects that remerge-diff did put into the tmp-objdir.\n> That seems a lot less likely to me (because the thing we're most worried\n> about is unrelated code that just happens to write while the tmp-objdir\n> is in place).\n>\n> > > In that sense this is kind of like the pretend_object_file() interface,\n> > > except that it's storing the objects on disk instead of in memory. Of\n> > > course another way of doing that would be to stuff the object data into\n> > > tempfiles and just put pointers into the in-memory cached_objects array.\n> > >\n> > > It's also not entirely foolproof (nor is the existing\n> > > pretend_object_file()). Any operation which is fooled into thinking we\n> > > have object X because it's in the fake-object list or the tmp_objdir may\n> > > reference that object erroneously, creating a corruption. But it's still\n> > > safer than allowing arbitrary writes into the tmp_objdir.\n> >\n> > Why is the pretend_object_file() interface safer? Does it disable the\n> > textconv caching somehow? I don't see why it helps avoid this\n> > problem.\n>\n> So hopefully it's clearer now from what I wrote above, but just\n> connecting the dots:\n>\n> 1. Unrelated code calling write_object_file() would still write real,\n> durable objects, as usual.\n>\n> 2. The write_object_file() \"skip this write\" optimization already\n> ignores the pretend_object_file() objects while checking \"do we\n> have this object\".\n>\n> > Interesting. I'm not familiar with the pretend_object_file() code,\n> > but I think I catch what you're saying. I don't see anything in\n> > object-file.c that allows removing items from the cache, as I would\n> > want to do, but I'm sure I could add something.\n>\n> Right, it's pretty limited now, and there's only one caller. And in fact\n> I've wanted to get rid of it, exactly because it can create the danger\n> we're discussing here. But I still think it's _less_ dangerous than\n> fully replacing the object directory.\n>\n> > (I'm still not sure how this avoids the problem of things like\n> > textconv caching trying to write an object that references one of\n> > these, though. Should we just manually disable textconv caching\n> > during a remerge-diff?)\n>\n> It can't avoid it completely, but in general, the textconv cache should\n> be writing and referencing its own objects. Even if they happen to be\n> the same as something remerge-diff generated, it won't know that until\n> it has tried to write it (and so that's why having write_object_file()\n> continue to really write the object is important).\n>\n>\n> Hopefully that clears up my line of thinking. I do think a lot of this\n> is kind of theoretical, in the sense that:\n>\n> - textconv caching is an obscure feature that we _could_ just disable\n> during remerge-diffs\n>\n> - there's a reasonable chance that there isn't any other code that\n> wants to write during a diff\n>\n> But this is all scary and error-prone enough that I'd prefer an approach\n> that has the \"least surprise\". So any solution where random code calling\n> write_object_file() _can't_ accidentally write to a throw-away directory\n> seems like a safer less surprising thing.\n\nYes, thanks for taking the time to go into this detail.\n\n> It does mean that the remerge-diff code needs to be explicit in its\n> object writes to say \"this needs to go to the temporary area\" (whether\n> it's a flag, or calling into a totally different function or subsystem).\n> I'm hoping that's not hard to do (because its writes are done explicitly\n> by the remerge-diff code), but I worry that it may be (because you are\n> relying on more generic tree code to the write under the hood).\n\nAll blob and tree objects written during merge-ort are done by\nexplicit write_object_file() calls that exist within merge-ort.c. So\nthis approach should be doable, it just needs the external caller to\nset some flag saying to write these objects elsewhere.\n"},{"id":"437602","messageId":"CABPp-BEqcq0wYqNP1xUJj8+E5HUTnip7+asadRoeFLAZ34rTdQ@mail.gmail.com","threadId":"56411","inReplyTo":"YVVrY0/kXfWHCWJ1@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T02:31:44Z","receivedAt":"2021-10-01T02:31:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 30, 2021 at 12:46 AM Jeff King wrote:\n>\n> On Thu, Sep 30, 2021 at 03:26:42AM -0400, Jeff King wrote:\n>\n> > > > If you remove the tmp_objdir as the primary as soon as you're done with\n> > > > the merge, but before you run the diff, you might be OK, though.\n> > >\n> > > It has to be after I run the diff, because the diff needs access to\n> > > the temporary files to diff against them.\n> >\n> > Right, of course. I was too fixated on the object-write part, forgetting\n> > that the whole point of the exercise is to later read them back. :)\n>\n> Ah, no, I remember what I was trying to say here. The distinction is\n> between \"remove the tmp_objdir\" and \"remove it as the primary\".\n>\n> I.e., if you do this:\n>\n> 1. create tmp_objdir\n>\n> 2. make tmp_objdir primary for writes\n>\n> 3. run the \"merge\" half of remerge-diff, writing objects into the\n> temporary space\n>\n> 4. stop having tmp_objdir as the primary; instead make it an alternate\n>\n> 5. run the diff\n>\n> 6. remove tmp_objdir totally\n>\n> Then step 5 can't accidentally write objects into the temporary space,\n> but it can still read them. So it's not entirely safe, but it's safer,\n> and it would be a much smaller change.\n\nInteresting.\n\n> Some ways it could go wrong:\n>\n> - is it possible for the merge code to ever write an object? I kind of\n> wonder if we'd ever do any cache-able transformations as part of a\n> content-level merge. I don't think we do now, though.\n\nYes, of course -- otherwise there would have been no need for the\ntmp_objdir in the first place. In particular, it needs to write\nthree-way-content merges of individual files, and it needs to write\nnew tree objects. (And it needs to do this both for creating the\nvirtual merge bases if the merge is recursive, as well as doing it for\nthe outer merge.)\n\nIt doesn't write anything for caching reasons, such as line ending\nnormalizations (that's all kept in-memory and done on demand).\n\n> - in step 5, write_object_file() may still be confused by the presence\n> of the to-be-thrown-away objects in the alternate. This is pretty\n> unlikely, as it implies that the remerge-diff wrote a blob or tree\n> that is byte-identical to something that the diff wants to write.\n\nThat's one reason it could be confused. The textconv filtering in\nparticular was creating a new object based on an existing one, and a\ntree, and a ref. What if there was some other form of caching or\nstatistic gathering that didn't write a new object based on an\nexisting one, but did add trees and especially refs that referenced\nthe existing object? It's not that the diff wanted to write something\nbyte-identical to what the merge wrote, it's just that the diff wants\nto reference the object somehow.\n"},{"id":"437604","messageId":"CABPp-BGsjq3ts4A6wKLYcopD9rknM+LXXi8qR_SLEpmU+x7KNQ@mail.gmail.com","threadId":"56411","inReplyTo":"87tui2tckn.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T03:11:59Z","receivedAt":"2021-10-01T03:12:14Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 30, 2021 at 6:31 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Thu, Sep 30 2021, Jeff King wrote:\n>\n> > On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:\n> >\n> >> Jeff King writes:\n> >>\n> >> > Side note: The pretend_object_file() approach is actually even better,\n> >> > because we know the object is fake. So it does not confuse\n> >> > write_object_file()'s \"do we already have this object\" freshening\n> >> > check.\n> >> >\n> >> > I suspect it could even be made faster than the tmp_objdir approach.\n> >> > From our perspective, these objects really are tempfiles. So we could\n> >> > write them as such, not worrying about things like fsyncing them,\n> >> > naming them into place, etc. We could just write them out, then mmap\n> >> > the results, and put the pointers into cached_objects (currently it\n> >> > insists on malloc-ing a copy of the input buffer, but that seems like\n> >> > an easy extension to add).\n> >> >\n> >> > In fact, I think you could get away with just _one_ tempfile per\n> >> > merge. Open up one tempfile. Write out all of the objects you want to\n> >> > \"store\" into it in sequence, and record the lseek() offsets before and\n> >> > after for each object. Then mmap the whole result, and stuff the\n> >> > appropriate pointers (based on arithmetic with the offsets) into the\n> >> > cached_objects list.\n> >>\n> >> Cute. The remerge diff code path creates a full tree that records\n> >> the mechanical merge result. By hooking into the lowest layer of\n> >> write_object() interface, we'd serialize all objects in such a tree\n> >> in the order they are computed (bottom up from the leaf level, I'd\n> >> presume) into a single flat file ;-)\n> >\n> > I do still like this approach, but just two possible gotchas I was\n> > thinking of:\n> >\n> > - This side-steps all of our usual code for getting object data into\n> > memory. In general, I'd expect this content to not be too enormous,\n> > but it _could_ be if there are many / large blobs in the result. So\n> > we may end up with large maps. Probably not a big deal on modern\n> > 64-bit systems. Maybe an issue on 32-bit systems, just because of\n> > virtual address space.\n> >\n> > Likewise, we do support systems with NO_MMAP. They'd work here, but\n> > it would probably mean putting all that object data into the heap. I\n> > could live with that, given how rare such systems are these days, and\n> > that it only matters if you're using --remerge-diff with big blobs.\n> >\n> > - I wonder to what degree --remerge-diff benefits from omitting writes\n> > for objects we already have. I.e., if you are writing out a whole\n> > tree representing the conflicted state, then you don't want to write\n> > all of the trees that aren't interesting. Hopefully the code is\n> > already figuring out which paths the merge even touched, and ignoring\n> > the rest. It probably benefits performance-wise from\n> > write_object_file() deciding to skip some object writes, as well\n> > (e.g., for resolutions which the final tree already took, as they'd\n> > be in the merge commit). The whole pretend-we-have-this-object thing\n> > may want to likewise make sure we don't write out objects that we\n> > already have in the real odb.\n>\n> I haven't benchmarked since my core.checkCollisions RFC patch[1]\n> resulted in the somewhat related loose object cache patch from you, and\n> not with something like the midx, but just a note that on some setups\n> just writing things out is faster than exhaustively checking if we\n> absolutely need to write things out.\n>\n> I also wonder how much if anything writing out the one file v.s. lots of\n> loose objects is worthwhile on systems where we could write out those\n> loose objects on a ramdisk, which is commonly available on e.g. Linux\n> distros these days out of the box. If you care about performance but not\n> about your transitory data using a ramdisk is generally much better than\n> any other potential I/O optimization.\n>\n> Finally, and I don't mean to throw a monkey wrench into this whole\n> discussion, so take this as a random musing: I wonder how much faster\n> this thing could be on its second run if instead of avoiding writing to\n> the store & cleaning up, it just wrote to the store, and then wrote\n\nIt'd be _much_ slower. My first implementation in fact did that; it\njust wrote objects to the store, left them there, and didn't bother to\ndo any auto-gcs. It slowed down quite a bit as it ran. Adding\nauto-gc's during the run were really slow too. But stepping back,\ngc'ing objects that I already knew were garbage seemed like a waste;\nwhy not just prune them pre-emptively? To do so, though, I'd have to\ntrack all the individual objects I added to make sure I didn't prune\nsomething else. Following that idea and a few different attempts\neventually led me to the discovery of tmp_objdir.\n\nIn case it's not clear to you why just writing all the objects to the\nnormal store and leaving them there slows things down so much...\n\nLet's say 1 in 10 merges had originally needed some kind of conflict\nresolution (either in the outer merge or in the inner merge for the\nvirtual merge bases), meaning that 9 out of 10 merges traversed by\n--remerge-diff don't write any objects. Now for each merge for which\n--remerge-diff does need to create conflicted blobs and new trees,\nlet's say it writes on average 3 blobs and 7 trees. (I don't know the\nreal average numbers, it could well be ~5 total, but ~10 seems a\nrealistic first order approximation and it makes the math easy.)\nThen, if we keep all objects we write, then `git log --remerge-diff`\non a history with 100,000 merge commits, will have added 100,000 loose\nobjects by the time it finishes. That means that all diff and merge\noperations slow down considerably as it runs due to all the extra\nloose objects.\n\n> another object keyed on the git version and any revision paramaters\n> etc., and then pretty much just had to do a \"git cat-file -p \"\n> to present the result to the user :)\n\nSo caching the full `git log ...` output, based on a hash of the\ncommand line flags, and then merely re-showing it later? And having\nthat output be invalidated as soon as any head advances? Or are you\nthinking of caching the output per-commit based on a hash of the other\ncommand line flags...potentially slowing non-log operations down with\nthe huge number of loose objects?\n\n> I suppose that would be throwing a lot more work at an eventual \"git gc\"\n> than we ever do now, so maybe it's a bit crazy, but I think it might be\n> an interesting direction in general to (ab)use either the primary or\n> some secondary store in the .git dir as a semi-permanent cache of\n> resolved queries from the likes of \"git log\".\n\nIf you do per-commit caching, and the user scrolls through enough\noutput (not hard to do, just searching the output for some string\noften is enough), that \"eventual\" git-gc will be the very next git\noperation. If you cache the entire output, it'll be invalidated\npretty quickly. So I don't see how this works. Or am I\nmisunderstanding something you're suggesting here?\n"},{"id":"437605","messageId":"CABPp-BF_KqEUXB8H7Xsjjk10sb1hB8Qt3AzAeURAdDSxHYDo9w@mail.gmail.com","threadId":"56411","inReplyTo":"87h7e2t9fh.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T03:55:54Z","receivedAt":"2021-10-01T03:56:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 30, 2021 at 7:39 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Tue, Sep 28 2021, Elijah Newren wrote:\n>\n> > On Tue, Sep 28, 2021 at 1:00 AM Ævar Arnfjörð Bjarmason\n> > wrote:\n> >>\n> >> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n> >>\n> >> I commented just now on how this API is duplicated between here &\n> >> another in-flight series in\n> >> https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/; Just\n> >> comments on this patch here:\n> >>\n> >> > diff --git a/tmp-objdir.c b/tmp-objdir.c\n> >> > index b8d880e3626..9f75a75d1c0 100644\n> >> > --- a/tmp-objdir.c\n> >> > +++ b/tmp-objdir.c\n> >> > @@ -288,7 +288,36 @@ const char **tmp_objdir_env(const struct tmp_objdir *t)\n> >> > return t->env.v;\n> >> > }\n> >> >\n> >> > +const char *tmp_objdir_path(struct tmp_objdir *t)\n> >> > +{\n> >> > + return t->path.buf;\n> >> > +}\n> >>\n> >> Not your fault & this pre-dates your patch, but FWIW I prefer our APIs\n> >> that don't have these \"hidden struct\" shenanigans (like say \"struct\n> >> string_list\") so a caller could just access this, we can just declare it\n> >> \"const\" appropriately.\n> >>\n> >> We're also all in-tree friends here, so having various accessors for no\n> >> reason other than to access struct members seems a bit too much.\n> >\n> > That's a fair point, but just to play counterpoint for a minute...\n> >\n> > FWIW, I dislike when our public facing APIs are polluted with all\n> > kinds of internal details. merge-recursive being a case in point.\n> > When writing merge-ort, although I had a struct with public fields,\n> > that struct also contained an opaque struct (pointer) within it to\n> > hide several private fields. (I would have liked to hide or remove a\n> > few more fields, but couldn't do so while the merge_recursive_options\n> > struct was shared between both merge-ort and merge-recursive.)\n> >\n> > That said, I agree it can certainly be overdone, and tmp_objdir is\n> > pretty simple. However, sometimes even in simple cases I like when\n> > folks make use of an opaque struct because it means folks put some\n> > effort into thinking more about the API that should be exposed.\n> > That's something we as a project have often overlooked in the past, as\n> > evidenced both by our one-shot usage mentality, and the existence of\n> > external projects like libgit2 attempting to correct this design\n> > shortcoming. I'd like git to move more towards being structured as a\n> > reusable library as well as a useful command-line tool.\n>\n> [This is somewhat of a continuation of\n> https://lore.kernel.org/git/87lf3etaih.fsf@evledraar.gmail.com/].\n>\n> I like our APIs being usable, but right now we're not promising any\n> maintenance of APIs that work the same as git v2.30.0 or whatever.\n>\n> I think some external users directly link to libgit.a, but they're\n> tracking whatever churn we have from release to release, just like\n> someone maintaining out-of-tree linux kernel drivers.\n>\n> For maintenance of git.git itself I think it's going to stay like that\n> for any foreseeable future[1].\n>\n> And that's before we even get into the practicalities of git's license\n> likely not winning over many (if any) libgit2 users. I think those users\n> are if anything migrating from libgit2 to git's plumbing CLI, or in some\n> cases folding what they needed into git.git itself (or just staying with\n> libgit2).\n>\n> So assuming all of that I really don't see the point in general of\n> having a \"foo\" field that has the equivalent of get_foo() and set_foo()\n> accessors in the context of git.git.\n>\n> That's something that's really useful if you're maintaining API and ABI\n> compatibility, but for us it's usually just verbosity. All the users are\n> in-tree, so just add \"const\" as appropriate, or perhaps we should give\n> the field a \"priv\" name or whatever.\n\nIt wasn't just a get or set accessors, and there was more involved\nthan just adding const:\n * There was a typechange from struct strbuf -> const char *; strbuf != char *\n * Having an API that _enforces_ accesses to be const provides more\nsafety. Relying on callers to remember to declare their variable\naccess as const provides virtually none.\n\nBut more importantly than either of the above, tmp_objdir was locked\ndown because it was too easy to make big mistakes with, as Peff\npointed out. That meant that folks would only use it through the API,\nand if folks tried to do unusual things, they'd do so by adding new\nAPI, which is easier to flag in code reviews and make sure the right\npeople get cc'ed than if folks just reach into some internals from\nsome other file. And the new API that I added resulted in a\ndiscussion that pointed out I didn't need to access that field at all\nand should instead create and delete the tmp_objdir after every merge.\n\n> Obviously not everything should be done that way, e.g. being able to\n> have trace2 start/end points for something you'd otherwise push/clear\n> from a \"struct string_list\" or whatever directly *is* usable, so it's\n> not black and white.\n>\n> I'm just saying thaht using convoluted patterns *just* to get in the way\n> of some hypothetical code that's going to mess with your internals can\n> make things harder in other areas. E.g. the init/release pattern via\n> macros I mentioned in the linked E-Mail above.\n\nFair points, but I just don't think that's relevant to the context\nhere; i.e. these aren't convoluted patterns *just* to get in the way.\n\nFurther, I don't see how this would adversely affect the *_INIT macros\nyou mentioned at all. tmp_objdir is an opaque struct so you can only\ndeclare a pointer to it. Pointers being initialized to NULL is\nhandled quite nicely by the *_INIT macros.\n\n> > [...]\n> >> In this particular case I hadn't understood on a previous reading of\n> >> tmp-objdir.c why this \"path\" isn't a statically allocated \"char\n> >> path[PATH_MAX]\", or a least we that hardcoded \"1024\" should be a\n> >> PATH_MAX, as it is in some other cases.\n> >\n> > Actually, PATH_MAX shouldn't be used:\n> >\n> > https://lore.kernel.org/git/7d1237c7-5a83-d766-7d93-5f0d59166067@web.de/\n> > https://lore.kernel.org/git/20180720180427.GE22486@sigill.intra.peff.net/\n>\n> Thanks. I didn't know about that. I've fixed (some of that) up in some\n> semi-related WIP work I've got.\n>\n> 1. Aside: I *do* think it would be really useful for us to expose a\n> stable C API eventually, using some of the same license\n> workarounds/shenanigans (depending on your view) as the Linux kernel\n> did. I.e. to say that \"we're GPLv2, but this small API is the\n> equivalent of our syscall interface\".\n>\n> Rather than that API being something that would look anything like\n> internal git.git users it would basically be a run_command() + Perl's\n> unpack/pack + output formatter on steroids.\n>\n> I.e. we'd be able to invoke say a 'git cat-file --batch' in the same\n> process as the caller by essentially calling cmd_cat_file()\n> directly. Just with some callback replacements for \"provide this on\n> stdin\" and \"this line accepts stdout\" without actually involving\n> different processes or I/O.\n>\n> You'd then have the equivalent of a --format output that would just\n> so happen to be in your OS's / C ABI's understood struct\n> format. Similar to \"perldoc -f pack\".\n>\n> The maintenance burden of such an API would be trivial. It would\n> basically be a handful of functions just to setup input/output\n> callbacks.\n>\n> Most of the interface would be a rather convoluted construction of\n> command-line parameters (like our own run_command() use), not ideal,\n> but most users are after structured data and eliminating process\n> overhead, and they'd get that.\n>\n> We would need to replace fprintf() and the like with some structured\n> formatting mechanism, but that would be useful for non-API users as\n> new commands learning a shiny new --format parameter. You could even\n> use such a --format to have a non-C-API user get the same struct\n> output, perhaps for direct mapping into memory, say if you didn't\n> trust the built-in/command in question to not leak memory yet.\n>\n> I don't know if this has been suggested before, just my 0.02. It's\n> also partially why I'm interested in making even-built-ins not leak\n> memory with some of my parallel SANITIZE=leak work.\n>\n> Once you've got that you can usually call the cmd_whatever() in a\n> loop without much effort, and once you've got that, well, see above\n> :)\n\nThis sounds really cool.\n"},{"id":"437606","messageId":"CABPp-BGgytcH5kBF_xuukz=FfEi0-076AsvCsW=1otycAOMbdQ@mail.gmail.com","threadId":"56411","inReplyTo":"xmqqbl49bzfy.fsf@gitster.g","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T03:59:46Z","receivedAt":"2021-10-01T04:00:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 30, 2021 at 1:06 PM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > - is it possible for the merge code to ever write an object? I kind of\n> > wonder if we'd ever do any cache-able transformations as part of a\n> > content-level merge. I don't think we do now, though.\n>\n> How is virtual merge base, result of an inner merge that recursively\n> merging two merge bases, fed to an outer merge as the ancestor?\n> Isn't it written as a tree object by the inner merge as a tree with\n> full of blob objects, so the outer merge does not have to treat a\n> merge base tree that is virtual any specially from a sole merge\n> base?\n\nYes. And that's not unique to the inner merges; it also writes new\nblob and tree objects for the outer merge, and then passes the\nresulting toplevel tree id out as the result.\n"},{"id":"437607","messageId":"CABPp-BF20vpC4m5V4JtS91fuA+PMiEN78J9OpVEPDpnKsTcP8A@mail.gmail.com","threadId":"56411","inReplyTo":"YVVoXJo3DlPQd1A3@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T04:26:37Z","receivedAt":"2021-10-01T04:26:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 30, 2021 at 12:33 AM Jeff King wrote:\n>\n> On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:\n>\n> > Jeff King writes:\n> >\n> > > Side note: The pretend_object_file() approach is actually even better,\n> > > because we know the object is fake. So it does not confuse\n> > > write_object_file()'s \"do we already have this object\" freshening\n> > > check.\n> > >\n> > > I suspect it could even be made faster than the tmp_objdir approach.\n> > > From our perspective, these objects really are tempfiles. So we could\n> > > write them as such, not worrying about things like fsyncing them,\n> > > naming them into place, etc. We could just write them out, then mmap\n> > > the results, and put the pointers into cached_objects (currently it\n> > > insists on malloc-ing a copy of the input buffer, but that seems like\n> > > an easy extension to add).\n> > >\n> > > In fact, I think you could get away with just _one_ tempfile per\n> > > merge. Open up one tempfile. Write out all of the objects you want to\n> > > \"store\" into it in sequence, and record the lseek() offsets before and\n> > > after for each object. Then mmap the whole result, and stuff the\n> > > appropriate pointers (based on arithmetic with the offsets) into the\n> > > cached_objects list.\n> >\n> > Cute. The remerge diff code path creates a full tree that records\n> > the mechanical merge result. By hooking into the lowest layer of\n> > write_object() interface, we'd serialize all objects in such a tree\n> > in the order they are computed (bottom up from the leaf level, I'd\n> > presume) into a single flat file ;-)\n>\n> I do still like this approach, but just two possible gotchas I was\n> thinking of:\n>\n> - This side-steps all of our usual code for getting object data into\n> memory. In general, I'd expect this content to not be too enormous,\n> but it _could_ be if there are many / large blobs in the result. So\n> we may end up with large maps. Probably not a big deal on modern\n> 64-bit systems. Maybe an issue on 32-bit systems, just because of\n> virtual address space.\n>\n> Likewise, we do support systems with NO_MMAP. They'd work here, but\n> it would probably mean putting all that object data into the heap. I\n> could live with that, given how rare such systems are these days, and\n> that it only matters if you're using --remerge-diff with big blobs.\n\nUm, I'm starting to get uncomfortable with this pretend_object stuff.\nPart of the reason that merge-ort isn't truly \"in memory\" despite\nattempting to do exactly that, was because for large enough repos with\nenough files modified on both sides, I wasn't comfortable assuming\nthat all new files from three-way content merges and all new trees fit\ninto memory. I'm sure we'd be fine with current-day linux kernel\nsized repos. No big deal. In fact, most merges probably don't add\nmore than a few dozen new files. But for microsoft-sized repos, and\nwith repos tending to grow over time, more so when the tools\nthemselves scale nicely (which we've all been working on enabling),\nmakes me worry there might be enough new objects within a single merge\n(especially given the recursive inner merges) that we might need to\nworry about this.\n\n> - I wonder to what degree --remerge-diff benefits from omitting writes\n> for objects we already have. I.e., if you are writing out a whole\n> tree representing the conflicted state, then you don't want to write\n> all of the trees that aren't interesting. Hopefully the code is\n> already figuring out which paths the merge even touched, and ignoring\n> the rest.\n\nNot only do you want to avoid writing all of the trees that aren't\ninteresting, you also want to avoid traversing into them in the first\nplace and avoid doing trivial file merges for each entry underneath.\nSadly, merge-recursive did anyway. Because renames and directory\nrenames can sometimes throw a big wrench in the desire to avoid\ntraversing into such directories. Figuring out how to avoid it was\nkind of tricky, but merge-ort definitely handles this when it can\nsafely do so; see the cover letter for my trivial directory resolution\nseries[1].\n\n[1] https://lore.kernel.org/git/pull.988.v4.git.1626841444.gitgitgadget@gmail.com/\n\n> It probably benefits performance-wise from\n> write_object_file() deciding to skip some object writes, as well\n> (e.g., for resolutions which the final tree already took, as they'd\n> be in the merge commit).\n\nYes, it will also benefit from that, but the much bigger side is\navoiding needlessly recursing into directories unchanged on (at least)\none side.\n\n> The whole pretend-we-have-this-object thing\n> may want to likewise make sure we don't write out objects that we\n> already have in the real odb.\n\nRight, so I'd have to copy the relevant logic from write_object_file()\n-- I think that means instead of write_object_file()'s calls to\nfreshen_packed_object() and freshen_loose_object() that I instead call\nfind_pack_entry() and make has_loose_object() in object-file.c not be\nstatic and then call it. Does that sound right?\n\nOf course, that's assuming we're okay with this pretend_object thing,\nwhich I'm starting to worry about.\n"},{"id":"437608","messageId":"YVaa650FehPmk0QD@coredump.intra.peff.net","threadId":"56411","inReplyTo":"CABPp-BEqcq0wYqNP1xUJj8+E5HUTnip7+asadRoeFLAZ34rTdQ@mail.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-01T05:21:47Z","receivedAt":"2021-10-01T05:21:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2021 at 07:31:44PM -0700, Elijah Newren wrote:\n\n> > Some ways it could go wrong:\n> >\n> > - is it possible for the merge code to ever write an object? I kind of\n> > wonder if we'd ever do any cache-able transformations as part of a\n> > content-level merge. I don't think we do now, though.\n> \n> Yes, of course -- otherwise there would have been no need for the\n> tmp_objdir in the first place. In particular, it needs to write\n> three-way-content merges of individual files, and it needs to write\n> new tree objects. (And it needs to do this both for creating the\n> virtual merge bases if the merge is recursive, as well as doing it for\n> the outer merge.)\n\nRight, sorry, I should have added: ...in addition to the merge-results\nwe're writing. What I'm getting at is whether there might be _other_\nparts of the code that the merge code would invoke. Say, if a\n.gitattribute caused us to convert a file's encoding in order to perform\na more semantic merge, and we wanted to store that result somehow (e.g.,\nin a similar cache).\n\nI don't think anything like that exists now, but I don't find it outside\nthe realm of possibility in the future. It's sort of the merge\nequivalent of \"textconv\"; we have external diff and external merge\ndrivers, but being able to convert contents and feed them to the regular\ntext diff/merge code simplifies things.\n\n> > - in step 5, write_object_file() may still be confused by the presence\n> > of the to-be-thrown-away objects in the alternate. This is pretty\n> > unlikely, as it implies that the remerge-diff wrote a blob or tree\n> > that is byte-identical to something that the diff wants to write.\n> \n> That's one reason it could be confused. The textconv filtering in\n> particular was creating a new object based on an existing one, and a\n> tree, and a ref. What if there was some other form of caching or\n> statistic gathering that didn't write a new object based on an\n> existing one, but did add trees and especially refs that referenced\n> the existing object? It's not that the diff wanted to write something\n> byte-identical to what the merge wrote, it's just that the diff wants\n> to reference the object somehow.\n\nYes, good point. It can help a little if the alternate-odb added by\ntmp_objdir was marked with a flag to say \"hey, this is temporary\". That\nwould help write_object_file() decide not to depend on it. But the\nproblem is so much wider than that that I think it will always be a bit\ndangerous; any code could say \"can I access this object\" and we don't\nknow if its intent is simply to read it, or to create a new object that\nreferences it.\n\n-Peff\n"},{"id":"437609","messageId":"YVacUCcl6USJSY5T@coredump.intra.peff.net","threadId":"56411","inReplyTo":"CABPp-BF20vpC4m5V4JtS91fuA+PMiEN78J9OpVEPDpnKsTcP8A@mail.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-01T05:27:44Z","receivedAt":"2021-10-01T05:27:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2021 at 09:26:37PM -0700, Elijah Newren wrote:\n\n> > - This side-steps all of our usual code for getting object data into\n> > memory. In general, I'd expect this content to not be too enormous,\n> > but it _could_ be if there are many / large blobs in the result. So\n> > we may end up with large maps. Probably not a big deal on modern\n> > 64-bit systems. Maybe an issue on 32-bit systems, just because of\n> > virtual address space.\n> >\n> > Likewise, we do support systems with NO_MMAP. They'd work here, but\n> > it would probably mean putting all that object data into the heap. I\n> > could live with that, given how rare such systems are these days, and\n> > that it only matters if you're using --remerge-diff with big blobs.\n> \n> Um, I'm starting to get uncomfortable with this pretend_object stuff.\n> Part of the reason that merge-ort isn't truly \"in memory\" despite\n> attempting to do exactly that, was because for large enough repos with\n> enough files modified on both sides, I wasn't comfortable assuming\n> that all new files from three-way content merges and all new trees fit\n> into memory. I'm sure we'd be fine with current-day linux kernel\n> sized repos. No big deal. In fact, most merges probably don't add\n> more than a few dozen new files. But for microsoft-sized repos, and\n> with repos tending to grow over time, more so when the tools\n> themselves scale nicely (which we've all been working on enabling),\n> makes me worry there might be enough new objects within a single merge\n> (especially given the recursive inner merges) that we might need to\n> worry about this.\n\nI do think we need to consider that the content might be larger than\nwill comfortably fit in memory. But the point of using mmap is that we\ndon't have to care. The OS is taking care of it for us (just like it\nwould in regular object files).\n\nThe question is just whether we're comfortable assuming that mmap\nexists if you're working on such a large repository. I'd guess that big\nrepos are pretty painful with out it (and again, I'm not even clear\nwhich systems Git runs on even lack mmap these days). So IMHO this isn't\nreally a blocker for going in this direction.\n\n> > The whole pretend-we-have-this-object thing\n> > may want to likewise make sure we don't write out objects that we\n> > already have in the real odb.\n> \n> Right, so I'd have to copy the relevant logic from write_object_file()\n> -- I think that means instead of write_object_file()'s calls to\n> freshen_packed_object() and freshen_loose_object() that I instead call\n> find_pack_entry() and make has_loose_object() in object-file.c not be\n> static and then call it. Does that sound right?\n\nYes. You _could_ call the same freshening functions, but I think since\nwe know that our objects are throw-away anyway, we do not even need to\nperform that freshening operation. I think just has_object_file() would\nbe sufficient for your needs.\n\n-Peff\n"},{"id":"437610","messageId":"YVacjhpgu1BwXzdk@coredump.intra.peff.net","threadId":"56411","inReplyTo":"CABPp-BFimGLnP331aFqLx0x-uQG7wHwSoN25fcYrPzWU93fn6Q@mail.gmail.com","subject":"Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-01T05:28:46Z","receivedAt":"2021-10-01T05:28:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 30, 2021 at 07:07:21PM -0700, Elijah Newren wrote:\n\n> > That said, I think it could also make sense to annotate the conflict\n> > files by giving them an unusual set of mode bits. That would be easier\n> > to recognize (and while real trees _could_ have silly modes, we do\n> > complain about them in fsck, so it shouldn't happen in practice).\n> \n> I tried giving things unusual mode bits in early versions of merge-ort\n> for other reasons. It doesn't work: canon_mode() will \"fix\" the\n> unusualness so there's no way for the reader to know that they had\n> unusual bits.\n\nAh, right, I totally forgot about that.\n\n> But, as you said later, avoiding these pseudo-files is going to be\n> cleaner anyway, so let's just do that.\n\nI'm quite happy if you're on board with that direction. :)\n\n-Peff\n"},{"id":"437617","messageId":"871r55tdgw.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"CABPp-BFyh239+7FLuUuoRJJbcE8NxHZABPTXPmSQhvRSPCiGvQ@mail.gmail.com","subject":"Re: [PATCH 4/7] merge-ort: capture and print ll-merge warnings in our preferred fashion","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T07:23:30Z","receivedAt":"2021-10-01T07:24:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 30 2021, Elijah Newren wrote:\n\n> On Thu, Sep 30, 2021 at 9:53 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>>\n>> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:\n>>\n>> > @@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' '\n>> > printf \"\\0\\0\" >binary-file &&\n>> > git add binary-file &&\n>> > git commit -m binary2 &&\n>> > - test_must_fail git merge F >merge.out 2>merge.err &&\n>> > - grep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.err\n>> > + if test \"$GIT_TEST_MERGE_ALGORITHM\" = ort\n>> > + then\n>> > + test_must_fail git merge F >merge.out &&\n>> > + grep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.out\n>> > + else\n>> > + test_must_fail git merge F >merge.out 2>merge.err &&\n>> > + grep \"Cannot merge binary files: binary-file (HEAD vs. F)\" merge.err\n>> > + fi\n>> > '\n>>\n>> To save readers from eyeballing if a single character has changed here,\n>> which I don't think it has, just do:\n>>\n>> if ...\n>> then\n>> cmd >actual\n>> else\n>> other cmd >actual\n>\n> Do you mean\n> other cmd >/dev/null 2>actual\n> ?\n\nYes, sorry. I was going for lazy pseudocode. I meant maybe the \"grep\"\ncan be shared between the two.\n"},{"id":"437618","messageId":"87wnmxry3e.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"CABPp-BGsjq3ts4A6wKLYcopD9rknM+LXXi8qR_SLEpmU+x7KNQ@mail.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T07:30:05Z","receivedAt":"2021-10-01T07:41:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 30 2021, Elijah Newren wrote:\n\n> On Thu, Sep 30, 2021 at 6:31 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> On Thu, Sep 30 2021, Jeff King wrote:\n>>\n>> > On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:\n>> >\n>> >> Jeff King writes:\n>> >>\n>> >> > Side note: The pretend_object_file() approach is actually even better,\n>> >> > because we know the object is fake. So it does not confuse\n>> >> > write_object_file()'s \"do we already have this object\" freshening\n>> >> > check.\n>> >> >\n>> >> > I suspect it could even be made faster than the tmp_objdir approach.\n>> >> > From our perspective, these objects really are tempfiles. So we could\n>> >> > write them as such, not worrying about things like fsyncing them,\n>> >> > naming them into place, etc. We could just write them out, then mmap\n>> >> > the results, and put the pointers into cached_objects (currently it\n>> >> > insists on malloc-ing a copy of the input buffer, but that seems like\n>> >> > an easy extension to add).\n>> >> >\n>> >> > In fact, I think you could get away with just _one_ tempfile per\n>> >> > merge. Open up one tempfile. Write out all of the objects you want to\n>> >> > \"store\" into it in sequence, and record the lseek() offsets before and\n>> >> > after for each object. Then mmap the whole result, and stuff the\n>> >> > appropriate pointers (based on arithmetic with the offsets) into the\n>> >> > cached_objects list.\n>> >>\n>> >> Cute. The remerge diff code path creates a full tree that records\n>> >> the mechanical merge result. By hooking into the lowest layer of\n>> >> write_object() interface, we'd serialize all objects in such a tree\n>> >> in the order they are computed (bottom up from the leaf level, I'd\n>> >> presume) into a single flat file ;-)\n>> >\n>> > I do still like this approach, but just two possible gotchas I was\n>> > thinking of:\n>> >\n>> > - This side-steps all of our usual code for getting object data into\n>> > memory. In general, I'd expect this content to not be too enormous,\n>> > but it _could_ be if there are many / large blobs in the result. So\n>> > we may end up with large maps. Probably not a big deal on modern\n>> > 64-bit systems. Maybe an issue on 32-bit systems, just because of\n>> > virtual address space.\n>> >\n>> > Likewise, we do support systems with NO_MMAP. They'd work here, but\n>> > it would probably mean putting all that object data into the heap. I\n>> > could live with that, given how rare such systems are these days, and\n>> > that it only matters if you're using --remerge-diff with big blobs.\n>> >\n>> > - I wonder to what degree --remerge-diff benefits from omitting writes\n>> > for objects we already have. I.e., if you are writing out a whole\n>> > tree representing the conflicted state, then you don't want to write\n>> > all of the trees that aren't interesting. Hopefully the code is\n>> > already figuring out which paths the merge even touched, and ignoring\n>> > the rest. It probably benefits performance-wise from\n>> > write_object_file() deciding to skip some object writes, as well\n>> > (e.g., for resolutions which the final tree already took, as they'd\n>> > be in the merge commit). The whole pretend-we-have-this-object thing\n>> > may want to likewise make sure we don't write out objects that we\n>> > already have in the real odb.\n>>\n>> I haven't benchmarked since my core.checkCollisions RFC patch[1]\n>> resulted in the somewhat related loose object cache patch from you, and\n>> not with something like the midx, but just a note that on some setups\n>> just writing things out is faster than exhaustively checking if we\n>> absolutely need to write things out.\n>>\n>> I also wonder how much if anything writing out the one file v.s. lots of\n>> loose objects is worthwhile on systems where we could write out those\n>> loose objects on a ramdisk, which is commonly available on e.g. Linux\n>> distros these days out of the box. If you care about performance but not\n>> about your transitory data using a ramdisk is generally much better than\n>> any other potential I/O optimization.\n>>\n>> Finally, and I don't mean to throw a monkey wrench into this whole\n>> discussion, so take this as a random musing: I wonder how much faster\n>> this thing could be on its second run if instead of avoiding writing to\n>> the store & cleaning up, it just wrote to the store, and then wrote\n>\n> It'd be _much_ slower. My first implementation in fact did that; it\n> just wrote objects to the store, left them there, and didn't bother to\n> do any auto-gcs. It slowed down quite a bit as it ran. Adding\n> auto-gc's during the run were really slow too. But stepping back,\n> gc'ing objects that I already knew were garbage seemed like a waste;\n> why not just prune them pre-emptively? To do so, though, I'd have to\n> track all the individual objects I added to make sure I didn't prune\n> something else. Following that idea and a few different attempts\n> eventually led me to the discovery of tmp_objdir.\n>\n> In case it's not clear to you why just writing all the objects to the\n> normal store and leaving them there slows things down so much...\n>\n> Let's say 1 in 10 merges had originally needed some kind of conflict\n> resolution (either in the outer merge or in the inner merge for the\n> virtual merge bases), meaning that 9 out of 10 merges traversed by\n> --remerge-diff don't write any objects. Now for each merge for which\n> --remerge-diff does need to create conflicted blobs and new trees,\n> let's say it writes on average 3 blobs and 7 trees. (I don't know the\n> real average numbers, it could well be ~5 total, but ~10 seems a\n> realistic first order approximation and it makes the math easy.)\n> Then, if we keep all objects we write, then `git log --remerge-diff`\n> on a history with 100,000 merge commits, will have added 100,000 loose\n> objects by the time it finishes. That means that all diff and merge\n> operations slow down considerably as it runs due to all the extra\n> loose objects.\n\n...\n\n>> another object keyed on the git version and any revision paramaters\n>> etc., and then pretty much just had to do a \"git cat-file -p \"\n>> to present the result to the user :)\n>\n> So caching the full `git log ...` output, based on a hash of the\n> command line flags, and then merely re-showing it later? And having\n> that output be invalidated as soon as any head advances? Or are you\n> thinking of caching the output per-commit based on a hash of the other\n> command line flags...potentially slowing non-log operations down with\n> the huge number of loose objects?\n\nYeah I meant caching the full 'git log' output. Anyway, I think what you\nsaid above about number of loose objects clearly makes that a stupid\nsuggestion on my part.\n\nFWIW I meant that more as a \"maybe in the future we can ...\" musing than\nanything for this series. I.e. in general I've often wanted say the\nsecond run of a 'git log -G\"' to be faster than it is, which could\nuse such on-the-fly caching.\n\nBut if we had such caching then something like --remerge-diff would need\nto (from my understanding of what you're saying) need both this\ntemporary staging area for objects and perhaps to cache the output in\nthe main (or some cache) store. I.e. they're orthagonal.\n\n>> I suppose that would be throwing a lot more work at an eventual \"git gc\"\n>> than we ever do now, so maybe it's a bit crazy, but I think it might be\n>> an interesting direction in general to (ab)use either the primary or\n>> some secondary store in the .git dir as a semi-permanent cache of\n>> resolved queries from the likes of \"git log\".\n>\n> If you do per-commit caching, and the user scrolls through enough\n> output (not hard to do, just searching the output for some string\n> often is enough), that \"eventual\" git-gc will be the very next git\n> operation. If you cache the entire output, it'll be invalidated\n> pretty quickly. So I don't see how this works. Or am I\n> misunderstanding something you're suggesting here?\n\nWith the caveat above that I think this is all a pretty stupid\nsuggestion on my part:\n\nJust on the narrow aspect of how \"git gc\" behaves in that scenario: We'd\nkeep those objects around for 2 weeks by default, or whatever you have\ngc.pruneExpire set to.\n\nSo such a setup would certainly cause lots of pathological issues (see\nmy [1] for one), but I don't think over-eager expiry of loose objects\nwould be one.\n\nI'm not sure but are you referring to \"invalidated pretty quickly\"\nbecause it would go over the gc.auto limit? If so that's now how it\nworks in this scenario, see [2]. I.e. it's moderated by the\ngc.pruneExpire setting.\n\n1. https://lore.kernel.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/\n2. https://lore.kernel.org/git/87inc89j38.fsf@evledraar.gmail.com/\n"},{"id":"437619","messageId":"87sfxlrxtk.fsf@evledraar.gmail.com","threadId":"56411","inReplyTo":"YVacUCcl6USJSY5T@coredump.intra.peff.net","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-01T07:43:48Z","receivedAt":"2021-10-01T07:47:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 01 2021, Jeff King wrote:\n\n> On Thu, Sep 30, 2021 at 09:26:37PM -0700, Elijah Newren wrote:\n>\n>> > - This side-steps all of our usual code for getting object data into\n>> > memory. In general, I'd expect this content to not be too enormous,\n>> > but it _could_ be if there are many / large blobs in the result. So\n>> > we may end up with large maps. Probably not a big deal on modern\n>> > 64-bit systems. Maybe an issue on 32-bit systems, just because of\n>> > virtual address space.\n>> >\n>> > Likewise, we do support systems with NO_MMAP. They'd work here, but\n>> > it would probably mean putting all that object data into the heap. I\n>> > could live with that, given how rare such systems are these days, and\n>> > that it only matters if you're using --remerge-diff with big blobs.\n>> \n>> Um, I'm starting to get uncomfortable with this pretend_object stuff.\n>> Part of the reason that merge-ort isn't truly \"in memory\" despite\n>> attempting to do exactly that, was because for large enough repos with\n>> enough files modified on both sides, I wasn't comfortable assuming\n>> that all new files from three-way content merges and all new trees fit\n>> into memory. I'm sure we'd be fine with current-day linux kernel\n>> sized repos. No big deal. In fact, most merges probably don't add\n>> more than a few dozen new files. But for microsoft-sized repos, and\n>> with repos tending to grow over time, more so when the tools\n>> themselves scale nicely (which we've all been working on enabling),\n>> makes me worry there might be enough new objects within a single merge\n>> (especially given the recursive inner merges) that we might need to\n>> worry about this.\n>\n> I do think we need to consider that the content might be larger than\n> will comfortably fit in memory. But the point of using mmap is that we\n> don't have to care. The OS is taking care of it for us (just like it\n> would in regular object files).\n>\n> The question is just whether we're comfortable assuming that mmap\n> exists if you're working on such a large repository. I'd guess that big\n> repos are pretty painful with out it (and again, I'm not even clear\n> which systems Git runs on even lack mmap these days). So IMHO this isn't\n> really a blocker for going in this direction.\n\nOn the not a blocker: Even without mmap() such a user also has the out\nof increasing the size of their swap/page file.\n\nAnd generally I agree that it's fair for us to say that if you've got\nsuch outsized performance needs you're going to need something more than\nthe lowest common denominator git can be ported to.\n\nI also wouldn't be surprised if in that scenario we'd run faster if we\nwere using memory (and no mmap) than if we tried to fallback to the FS,\ni.e. your suggestion of replacing N loose objects with one file with\nindex offsets is basically what the OS would be doing for you as it\npages out memory it can't fit in RAM to disk.\n"},{"id":"437621","messageId":"CABPp-BEh3w8DxyLuc5GtowFKeNx7h7joDarUfoFjXihvMEzp-g@mail.gmail.com","threadId":"56411","inReplyTo":"87wnmxry3e.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-10-01T08:03:57Z","receivedAt":"2021-10-01T08:04:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Oct 1, 2021 at 12:41 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Thu, Sep 30 2021, Elijah Newren wrote:\n>\n> > On Thu, Sep 30, 2021 at 6:31 AM Ævar Arnfjörð Bjarmason\n> > wrote:\n> >>\n> >> On Thu, Sep 30 2021, Jeff King wrote:\n> >>\n> >> > On Tue, Sep 28, 2021 at 09:08:00PM -0700, Junio C Hamano wrote:\n> >> >\n> >> >> Jeff King writes:\n> >> >>\n> >> >> > Side note: The pretend_object_file() approach is actually even better,\n> >> >> > because we know the object is fake. So it does not confuse\n> >> >> > write_object_file()'s \"do we already have this object\" freshening\n> >> >> > check.\n> >> >> >\n> >> >> > I suspect it could even be made faster than the tmp_objdir approach.\n> >> >> > From our perspective, these objects really are tempfiles. So we could\n> >> >> > write them as such, not worrying about things like fsyncing them,\n> >> >> > naming them into place, etc. We could just write them out, then mmap\n> >> >> > the results, and put the pointers into cached_objects (currently it\n> >> >> > insists on malloc-ing a copy of the input buffer, but that seems like\n> >> >> > an easy extension to add).\n> >> >> >\n> >> >> > In fact, I think you could get away with just _one_ tempfile per\n> >> >> > merge. Open up one tempfile. Write out all of the objects you want to\n> >> >> > \"store\" into it in sequence, and record the lseek() offsets before and\n> >> >> > after for each object. Then mmap the whole result, and stuff the\n> >> >> > appropriate pointers (based on arithmetic with the offsets) into the\n> >> >> > cached_objects list.\n> >> >>\n> >> >> Cute. The remerge diff code path creates a full tree that records\n> >> >> the mechanical merge result. By hooking into the lowest layer of\n> >> >> write_object() interface, we'd serialize all objects in such a tree\n> >> >> in the order they are computed (bottom up from the leaf level, I'd\n> >> >> presume) into a single flat file ;-)\n> >> >\n> >> > I do still like this approach, but just two possible gotchas I was\n> >> > thinking of:\n> >> >\n> >> > - This side-steps all of our usual code for getting object data into\n> >> > memory. In general, I'd expect this content to not be too enormous,\n> >> > but it _could_ be if there are many / large blobs in the result. So\n> >> > we may end up with large maps. Probably not a big deal on modern\n> >> > 64-bit systems. Maybe an issue on 32-bit systems, just because of\n> >> > virtual address space.\n> >> >\n> >> > Likewise, we do support systems with NO_MMAP. They'd work here, but\n> >> > it would probably mean putting all that object data into the heap. I\n> >> > could live with that, given how rare such systems are these days, and\n> >> > that it only matters if you're using --remerge-diff with big blobs.\n> >> >\n> >> > - I wonder to what degree --remerge-diff benefits from omitting writes\n> >> > for objects we already have. I.e., if you are writing out a whole\n> >> > tree representing the conflicted state, then you don't want to write\n> >> > all of the trees that aren't interesting. Hopefully the code is\n> >> > already figuring out which paths the merge even touched, and ignoring\n> >> > the rest. It probably benefits performance-wise from\n> >> > write_object_file() deciding to skip some object writes, as well\n> >> > (e.g., for resolutions which the final tree already took, as they'd\n> >> > be in the merge commit). The whole pretend-we-have-this-object thing\n> >> > may want to likewise make sure we don't write out objects that we\n> >> > already have in the real odb.\n> >>\n> >> I haven't benchmarked since my core.checkCollisions RFC patch[1]\n> >> resulted in the somewhat related loose object cache patch from you, and\n> >> not with something like the midx, but just a note that on some setups\n> >> just writing things out is faster than exhaustively checking if we\n> >> absolutely need to write things out.\n> >>\n> >> I also wonder how much if anything writing out the one file v.s. lots of\n> >> loose objects is worthwhile on systems where we could write out those\n> >> loose objects on a ramdisk, which is commonly available on e.g. Linux\n> >> distros these days out of the box. If you care about performance but not\n> >> about your transitory data using a ramdisk is generally much better than\n> >> any other potential I/O optimization.\n> >>\n> >> Finally, and I don't mean to throw a monkey wrench into this whole\n> >> discussion, so take this as a random musing: I wonder how much faster\n> >> this thing could be on its second run if instead of avoiding writing to\n> >> the store & cleaning up, it just wrote to the store, and then wrote\n> >\n> > It'd be _much_ slower. My first implementation in fact did that; it\n> > just wrote objects to the store, left them there, and didn't bother to\n> > do any auto-gcs. It slowed down quite a bit as it ran. Adding\n> > auto-gc's during the run were really slow too. But stepping back,\n> > gc'ing objects that I already knew were garbage seemed like a waste;\n> > why not just prune them pre-emptively? To do so, though, I'd have to\n> > track all the individual objects I added to make sure I didn't prune\n> > something else. Following that idea and a few different attempts\n> > eventually led me to the discovery of tmp_objdir.\n> >\n> > In case it's not clear to you why just writing all the objects to the\n> > normal store and leaving them there slows things down so much...\n> >\n> > Let's say 1 in 10 merges had originally needed some kind of conflict\n> > resolution (either in the outer merge or in the inner merge for the\n> > virtual merge bases), meaning that 9 out of 10 merges traversed by\n> > --remerge-diff don't write any objects. Now for each merge for which\n> > --remerge-diff does need to create conflicted blobs and new trees,\n> > let's say it writes on average 3 blobs and 7 trees. (I don't know the\n> > real average numbers, it could well be ~5 total, but ~10 seems a\n> > realistic first order approximation and it makes the math easy.)\n> > Then, if we keep all objects we write, then `git log --remerge-diff`\n> > on a history with 100,000 merge commits, will have added 100,000 loose\n> > objects by the time it finishes. That means that all diff and merge\n> > operations slow down considerably as it runs due to all the extra\n> > loose objects.\n>\n> ...\n>\n> >> another object keyed on the git version and any revision paramaters\n> >> etc., and then pretty much just had to do a \"git cat-file -p \"\n> >> to present the result to the user :)\n> >\n> > So caching the full `git log ...` output, based on a hash of the\n> > command line flags, and then merely re-showing it later? And having\n> > that output be invalidated as soon as any head advances? Or are you\n> > thinking of caching the output per-commit based on a hash of the other\n> > command line flags...potentially slowing non-log operations down with\n> > the huge number of loose objects?\n>\n> Yeah I meant caching the full 'git log' output. Anyway, I think what you\n> said above about number of loose objects clearly makes that a stupid\n> suggestion on my part.\n\nMost my ideas for merge-ort were stupid. I didn't know until I tried\nthem, and then I just discarded them and only showed off the good\nideas. :-)\n\nBrainstorming new ways of doing things is a useful exercise, I think.\n\n> FWIW I meant that more as a \"maybe in the future we can ...\" musing than\n> anything for this series. I.e. in general I've often wanted say the\n> second run of a 'git log -G\"' to be faster than it is, which could\n> use such on-the-fly caching.\n>\n> But if we had such caching then something like --remerge-diff would need\n> to (from my understanding of what you're saying) need both this\n> temporary staging area for objects and perhaps to cache the output in\n> the main (or some cache) store. I.e. they're orthagonal.\n>\n> >> I suppose that would be throwing a lot more work at an eventual \"git gc\"\n> >> than we ever do now, so maybe it's a bit crazy, but I think it might be\n> >> an interesting direction in general to (ab)use either the primary or\n> >> some secondary store in the .git dir as a semi-permanent cache of\n> >> resolved queries from the likes of \"git log\".\n> >\n> > If you do per-commit caching, and the user scrolls through enough\n> > output (not hard to do, just searching the output for some string\n> > often is enough), that \"eventual\" git-gc will be the very next git\n> > operation. If you cache the entire output, it'll be invalidated\n> > pretty quickly. So I don't see how this works. Or am I\n> > misunderstanding something you're suggesting here?\n>\n> With the caveat above that I think this is all a pretty stupid\n> suggestion on my part:\n>\n> Just on the narrow aspect of how \"git gc\" behaves in that scenario: We'd\n> keep those objects around for 2 weeks by default, or whatever you have\n> gc.pruneExpire set to.\n>\n> So such a setup would certainly cause lots of pathological issues (see\n> my [1] for one), but I don't think over-eager expiry of loose objects\n> would be one.\n>\n> I'm not sure but are you referring to \"invalidated pretty quickly\"\n> because it would go over the gc.auto limit? If so that's now how it\n> works in this scenario, see [2]. I.e. it's moderated by the\n> gc.pruneExpire setting.\n\nBy \"invalidated pretty quickly\" I meant that if the user ran \"git log\n--all\" and you cached the entire output in a single cache file, then\nas soon as any ref is updated, that cached output is invalid because\n\"git log --all\" should now show different output than before.\n\nIt seems to be a case of pick your poison: either (a) cache output per\ncommit so that it can be re-used, and suffer from extreme numbers of\nloose objects, OR (b) cache the entire output to avoid the large\nnumber of loose objects, resulting in the cache only being useful if\nthe user passed the _exact_ same arguments and revisions to git-log --\nand also having the cache become stale as soon as any involved ref is\nupdated.\n\n>\n> 1. https://lore.kernel.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/\n> 2. https://lore.kernel.org/git/87inc89j38.fsf@evledraar.gmail.com/\n"},{"id":"437710","messageId":"xmqqczoo8zxn.fsf@gitster.g","threadId":"56411","inReplyTo":"CABPp-BGgytcH5kBF_xuukz=FfEi0-076AsvCsW=1otycAOMbdQ@mail.gmail.com","subject":"Re: [PATCH 5/7] tmp-objdir: new API for creating and removing primary object dirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-01T16:36:36Z","receivedAt":"2021-10-01T16:36:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Thu, Sep 30, 2021 at 1:06 PM Junio C Hamano wrote:\n>>\n>> Jeff King writes:\n>>\n>> > - is it possible for the merge code to ever write an object? I kind of\n>> > wonder if we'd ever do any cache-able transformations as part of a\n>> > content-level merge. I don't think we do now, though.\n>>\n>> How is virtual merge base, result of an inner merge that recursively\n>> merging two merge bases, fed to an outer merge as the ancestor?\n>> Isn't it written as a tree object by the inner merge as a tree with\n>> full of blob objects, so the outer merge does not have to treat a\n>> merge base tree that is virtual any specially from a sole merge\n>> base?\n>\n> Yes. And that's not unique to the inner merges; it also writes new\n> blob and tree objects for the outer merge, and then passes the\n> resulting toplevel tree id out as the result.\n\nI think Peff's question was \"other than what is contained in the\nmerge result, are there objects we create during the merge process\nthat we may want to keep?\" And my inner merge example was objects\nthat do not appear in the merge result, but they are discardable,\nso it wasn't a good example.\n\nA tentative merge to recreate what the manual resolver would have\nseen while showing remerge-diff needs to be recorded somewhere\nbefore the diff gets shown, but after showing the diff, we can lose\nthem unless we need them for some other reason (e.g. they already\nexisted, another process happened to have created the same blob\nwhile we are running remerge-diff, etc.). So in that sense, the\nobjects that represent the result of the outermost merge is also\ndiscardable.\n\nThe question is if there are other things similar to the textconv\ncache, where we need to create during the operation and may keep the\nresult for later use. The closest thing I can think of is the\nrename cache we've talked about from time to time without actually\nadding one ;-).\n\nI guess using cached textconv result, without putting newly\ndiscovered textconv result back to the cache, would be the best we\ncan do?\n\nThe blobs (hence their textconv results) involved in showing a merge\nM with --remerge-diff are the ones in the trees of M^@ (all parents)\nand $(git merge-base $(git rev-parse M^@)), and they may have been\ncreated when running \"git show\" on or when letting \"git log -p\"\npass, these commits, so it is not like we are disabling the cache\nentirely, and allowing read-only access may still have value.\n\nThanks.\n\n"}]}