From: Elijah Newren via GitGitGadget <hidden> Date: 2021-08-31 02:26:46
Here are some patches to add a --remerge-diff capability to show & log,
which works by comparing merge commits to an automatic remerge (note that
the automatic remerge tree can contain files with conflict markers).
Here are some example commits you can try this out on (with git show
--remerge-diff $COMMIT):
* git.git conflicted merge: 07601b5b36
* git.git non-conflicted change: bf04590ecd
* linux.git conflicted merge: eab3540562fb
* linux.git non-conflicted change: 223cea6a4f05
Many more can be found by just running git log --merges --remerge-diff in
your repository of choice and searching for diffs (most merges tend to be
clean and unmodified and thus produce no diff but a search of '^diff' in the
log output tends to find the examples nicely).
Some basic high level details about this new option:
* This option is most naturally compared to --cc, though the output seems
to be much more understandable to most users than --cc output.
* Since merges are often clean and unmodified, this new option results in
an empty diff for most merges.
* This new option shows things like the removal of conflict markers, which
hunks users picked from the various conflicted sides to keep or remove,
and shows changes made outside of conflict markers (which might reflect
changes needed to resolve semantic conflicts or cleanups of e.g.
compilation warnings or other additional changes an integrator felt
belonged in the merged result).
* This new option does not (currently) work for octopus merges, since
merge-ort is specific to two-parent merges[1].
* This option will not work on a read-only or full filesystem[2].
* We discussed this capability at Git Merge 2020, and one of the
suggestions was doing a periodic git gc --auto during the operation (due
to potential new blobs and trees created during the operation). I found a
way to avoid that; see [2].
* This option is faster than you'd probably expect; it handles 33.5 merge
commits per second in linux.git on my computer; see below.
In regards to the performance point above, the timing for running the
following command:
time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l
in linux.git (with v5.4 checked out, since my copy of linux is very out of
date) is as follows:
DIFF_FLAG=--cc: 71m 31.536s
DIFF_FLAG=--remerge-diff: 31m 3.170s
Note that there are 62476 merges in this history. Also, output size is:
DIFF_FLAG=--cc: 2169111 lines
DIFF_FLAG=--remerge-diff: 2458020 lines
So roughly the same amount of output as --cc, as you'd expect.
As a side note: git log --remerge-diff, when run in various repositories and
allowed to run all the way back to the beginning(s) of history, is a nice
stress test of sorts for merge-ort. Especially when users run it for you on
their repositories they are working on, whether intentionally or via a bug
in a tool triggering that command to be run unexpectedly. Long story short,
such a bug in an internal tool existed last December and this command was
run on an internal repository and found a platform-specific bug in merge-ort
on some really old merge commit from that repo. I fixed that bug (a
STABLE_QSORT thing) while upstreaming all the merge-ort patches in the mean
time, but it was nice getting extra testing. Having more folks run this on
their repositories might be useful extra testing of the new merge strategy.
Also, I previously mentioned --remerge-diff-only (a flag to show how
cherry-picks or reverts differ from an automatic cherry-pick or revert, in
addition to showing how merges differ from an automatic merge). This series
does not include the patches to introduce that option; I'll submit them
later.
Two other things that might be interesting but are not included and which I
haven't investigated:
* some mechanism for passing extra merge options through (e.g.
-Xignore-space-change)
* a capability to compare the automatic merge to a second automatic merge
done with different merge options. (Not sure if this would be of interest
to end users, but might be interesting while developing new a
--strategy-option, or maybe checking how changing some default in the
merge algorithm would affect historical merges in various repositories).
[1] I have nebulous ideas of how an Octopus-centric ORT strategy could be
written -- basically, just repeatedly invoking ort and trying to make sure
nested conflicts can be differentiated. For now, though, a simple warning is
printed that octopus merges are not handled and no diff will be shown. [2]
New blobs/trees can be written by the three-way merging step. These are
written to a temporary area (via tmp-objdir.c) under the git object store
that is cleaned up at the end of the operation, with the new loose objects
from the remerge being cleaned up after each individual merge.
Elijah Newren (7):
merge-ort: mark a few more conflict messages as omittable
merge-ort: add ability to record conflict messages in a file
ll-merge: add API for capturing warnings in a strbuf instead of stderr
merge-ort: capture and print ll-merge warnings in our preferred
fashion
tmp-objdir: new API for creating and removing primary object dirs
show, log: provide a --remerge-diff capability
doc/diff-options: explain the new --remerge-diff option
Documentation/diff-options.txt | 8 +++
builtin/log.c | 23 ++++++++
diff-merges.c | 12 +++++
ll-merge.c | 51 +++++++++++++-----
ll-merge.h | 9 ++++
log-tree.c | 69 ++++++++++++++++++++++++
merge-ort.c | 96 +++++++++++++++++++++++++++++++---
merge-recursive.c | 3 ++
merge-recursive.h | 1 +
revision.h | 6 ++-
t/t6404-recursive-merge.sh | 10 +++-
t/t6406-merge-attr.sh | 10 +++-
tmp-objdir.c | 29 ++++++++++
tmp-objdir.h | 16 ++++++
14 files changed, 319 insertions(+), 24 deletions(-)
base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1080%2Fnewren%2Fremerge-diff-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1080/newren/remerge-diff-v1
Pull-Request: https://github.com/git/git/pull/1080
--
gitgitgadget
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-08-31 02:26:47
From: Elijah Newren <redacted>
path_msg() has the ability to mark messages as omittable when recording
conflict messages in an in-tree file. This conflict message file will
then be shown by diffing the merge-created tree to the actual merge
commit that is created. While all the messages touched in this commit
are very useful when trying to create a merge initially, early use with
the --remerge-diff feature (the only user of this omittable conflict
message capability), suggests that the particular messages marked in
this commit are just noise when trying to see what changes users made to
create a merge commit. Mark them as omittable.
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -2409,7 +2409,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,*/ci->path_conflict=1;if(pair->status=='A')-path_msg(opt,new_path,0,+path_msg(opt,new_path,1,_("CONFLICT (file location): %s added in %s ""inside a directory that was renamed in %s, ""suggesting it should perhaps be moved to "
@@ -2417,7 +2417,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,old_path,branch_with_new_path,branch_with_dir_rename,new_path);else-path_msg(opt,new_path,0,+path_msg(opt,new_path,1,_("CONFLICT (file location): %s renamed to %s ""in %s, inside a directory that was renamed ""in %s, suggesting it should perhaps be "
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-08-31 02:26:48
From: Elijah Newren <redacted>
We want to add an ability for users to run
git show --remerge-diff $MERGE_COMMIT
or even
git log -p --remerge-diff ...
and have git show the differences between where the merge machinery
would stop and what is recorded in merge commits. However, in such
cases, stdout is not an appropriate location to dump conflict messages.
Write those messages to a file in the merge result (not to the working
tree or index, but just to a blob recorded in the relevant tree object).
There are several considerations here:
* We have to pick file(s) where we write these conflict messages too
* We want to make it as clear as possible that it's not a real file
but a set of messages about another file
* We want conflict messages about a file to appear near the file in
question in a diff, preferably immediately preceding the file in
question
* Related to the above, per-file conflict messages are preferred
over lumping all conflict messages into one big file
To achive the above:
* We put the conflict messages for $filename in
$filename[0:-1] + " " + $filename[-1] + ".conflict_msg"
or, in words, we insert a space before the final character of
the filename and then also add ".conflict_msg" at the end.
* We start the file with a "== Conflict notices for $filename =="
banner
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++-
merge-recursive.c | 3 ++
merge-recursive.h | 1 +
3 files changed, 82 insertions(+), 1 deletion(-)
@@ -632,7 +632,11 @@ static void path_msg(struct merge_options *opt,constchar*fmt,...){va_listap;-structstrbuf*sb=strmap_get(&opt->priv->output,path);+structstrbuf*sb;++if(opt->record_conflict_msgs_in_tree&&omittable_hint)+return;/* Do not record mere hints in tree */+sb=strmap_get(&opt->priv->output,path);if(!sb){sb=xmalloc(sizeof(*sb));strbuf_init(sb,0);
@@ -3531,6 +3535,74 @@ static void write_completed_directory(struct merge_options *opt,info->last_directory_len=strlen(info->last_directory);}+staticvoidput_path_msgs_in_file(structmerge_options*opt,+constchar*path,+structmerged_info*mi,+structdirectory_versions*dir_metadata)+{+structstrbuftmp=STRBUF_INIT,new_path_contents=STRBUF_INIT;+char*new_path;+intnew_path_basic_len,unique_counter;+structmerged_info*new_mi;+charfinal;+structstrbuf*sb=strmap_get(&opt->priv->output,path);++assert(opt->record_conflict_msgs_in_tree);+if(!sb)+return;++/*+*Determineapathnameforrecordingconflictmessages.We'dlikeit+*tosortjustbeforepath,buthaveanameverysimilartowhatpath+*has.+*/+strbuf_addstr(&tmp,path);+final=tmp.buf[tmp.len-1];+strbuf_setlen(&tmp,tmp.len-1);+strbuf_addf(&tmp," %c.conflict_msg",final);++/*+*Inextremelyunlikelyeventthisfilenameisnotunique,modifyit+*with".<integer>"suffixesuntilitis.+*/+new_path_basic_len=tmp.len;+unique_counter=0;+while(strmap_contains(&opt->priv->paths,tmp.buf)){+strbuf_setlen(&tmp,new_path_basic_len);+strbuf_addf(&tmp,".%d",++unique_counter);+}++/* Now that we have a unique string, move it to our pool */+new_path=mem_pool_strdup(&opt->priv->pool,tmp.buf);+strbuf_release(&tmp);++/* Set up contents we want to place in the file. */+strbuf_addf(&new_path_contents,"== Conflict notices for %s ==\n",+path);+strbuf_addbuf(&new_path_contents,sb);++/* Set up new_mi */+new_mi=mem_pool_calloc(&opt->priv->pool,1,sizeof(*new_mi));+new_mi->result.mode=0100644;+new_mi->is_null=0;+new_mi->clean=1;+new_mi->basename_offset=mi->basename_offset;+new_mi->directory_name=mi->directory_name;+if(write_object_file(new_path_contents.buf,new_path_contents.len,+blob_type,&new_mi->result.oid))+die(_("Unable to add %s to database"),new_path);++/*+*Savenew_miinopt->priv->path(sothatsomethingwilldeallocate+*itlater),andrecordtheentryforit.+*/+strmap_put(&opt->priv->paths,new_path,new_mi);+record_entry_for_tree(dir_metadata,new_path,new_mi);++/* Cleanup */+strbuf_release(&new_path_contents);+}+/* Per entry merge function */staticvoidprocess_entry(structmerge_options*opt,constchar*path,
@@ -4226,6 +4300,9 @@ void merge_switch_to_result(struct merge_options *opt,structstring_listolist=STRING_LIST_INIT_NODUP;inti;+if(opt->record_conflict_msgs_in_tree)+BUG("Either display conflict messages or record them in tree, not both");+trace2_region_enter("merge","display messages",opt->repo);/* Hack to pre-allocate olist to the desired size */
@@ -3697,6 +3697,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)assert(opt->priv==NULL);+/* Not supported; option specific to merge-ort */+assert(!opt->record_conflict_msgs_in_tree);+/* Sanity check on repo state; index must match head */if(repo_index_has_changes(opt->repo,head,&sb)){err(opt,_("Your local changes to the following files would be overwritten by merge:\n %s"),
@@ -46,6 +46,7 @@ struct merge_options {/* miscellaneous control options */constchar*subtree_shift;unsignedrenormalize:1;+unsignedrecord_conflict_msgs_in_tree:1;/* internal fields used by the implementation */structmerge_options_internal*priv;
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-08-31 02:26:50
From: Elijah Newren <redacted>
Whenever ll-merge encounters a binary file, it prints a warning to
stderr. However, for the --remerge-diff option we want to add, we need
to capture all conflict messages and show them in a diff instead of
dumping them straight to stdout or stderr. Add some new API that will
allow us to capture this output and display it in our preferred method.
Signed-off-by: Elijah Newren <redacted>
---
ll-merge.c | 51 +++++++++++++++++++++++++++++++++++++++------------
ll-merge.h | 9 +++++++++
2 files changed, 48 insertions(+), 12 deletions(-)
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-08-31 02:26:51
From: Elijah Newren <redacted>
Instead of immediately printing ll-merge warnings to stderr, we save
them in our output strbuf. Besides allowing us to move these warnings
to a special file for --remerge-diff, this has two other benefits for
regular merges done by merge-ort:
* The deferral of messages ensures we can print all messages about
any given path together (merge-recursive was known to sometimes
intersperse messages about other paths, particularly when renames
were involved).
* The deferral of messages means we can avoid printing spurious
conflict messages when we just end up aborting due to local user
modifications in the way. (In contrast to merge-recursive.c which
prematurely checks for local modifications in the way via
unpack_trees() and gets the check wrong both in terms of false
positives and false negatives relative to renames, merge-ort does
not perform the local modifications in the way check until the
checkout() step after the full merge has been computed.)
Signed-off-by: Elijah Newren <redacted>
---
merge-ort.c | 11 ++++++++---
t/t6404-recursive-merge.sh | 10 ++++++++--
t/t6406-merge-attr.sh | 10 ++++++++--
3 files changed, 24 insertions(+), 7 deletions(-)
@@ -221,8 +221,14 @@ test_expect_success 'binary files with union attribute' 'printf"two\0">bin.txt&&gitcommit-amtwo&&-test_must_failgitmergebin-main2>stderr&&-grep-i"warning.*cannot merge.*HEAD vs. bin-main"stderr+iftest"$GIT_TEST_MERGE_ALGORITHM"=ort+then+test_must_failgitmergebin-main>stdout&&+grep-i"warning.*cannot merge.*HEAD vs. bin-main"stdout+else+test_must_failgitmergebin-main2>stderr&&+grep-i"warning.*cannot merge.*HEAD vs. bin-main"stderr+fi' test_done
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-08-31 02:26:52
From: Elijah Newren <redacted>
The tmp_objdir API provides the ability to create temporary object
directories, but was designed with the goal of having subprocesses
access these object stores, followed by the main process migrating
objects from it to the main object store or just deleting it. The
subprocesses would view it as their primary datastore and write to it.
For the --remerge-diff option we want to add to show & log, we want all
writes of intermediate merge results (both blobs and trees) to go to
this alternate object store; since those writes will be done by the main
process, we need this "alternate" object store to actually be the
primary object store. When show & log are done, they'll simply remove
this temporary object store.
We also need one more thing -- `git log --remerge-diff` can cause the
temporary object store to fill up with loose objects. Rather than
trying to gc that are known garbage anyway, we simply want to know the
location of the temporary object store so we can purge the loose objects
after each merge.
Signed-off-by: Elijah Newren <redacted>
---
tmp-objdir.c | 29 +++++++++++++++++++++++++++++
tmp-objdir.h | 16 ++++++++++++++++
2 files changed, 45 insertions(+)
@@ -64,6 +64,14 @@ ifdef::git-log[] each of the parents. Separate log entry and diff is generated for each parent. ++--diff-merges=remerge:::+--diff-merges=r:::+--remerge-diff:::+ With this option, two-parent merge commits are remerged to+ create a temporary tree object -- potentially containing files+ with conflict markers and such. A diff is then shown between+ that temporary tree and the actual merge commit.++ --diff-merges=combined::: --diff-merges=c::: -c:::
From: Elijah Newren via GitGitGadget <hidden> Date: 2021-08-31 02:26:54
From: Elijah Newren <redacted>
When this option is specified, we remerge all (two parent) merge commits
and diff the actual merge commit to the automatically created version,
in order to show how users removed conflict markers, resolved the
different conflict versions, and potentially added new changes outside
of conflict regions in order to resolve semantic merge problems (or,
possibly, just to hide other random changes).
This capability works by creating a temporary object directory and
marking it as the primary object store, so that any blobs or trees
created during the automatic merge, can be easily removed afterwards by
just deleting all objects from the temporary object directory. We can
do this after handling each merge commit, in order to avoid the need to
worry about doing `git gc --auto` runs while running `git log
--remerge-diff`.
Signed-off-by: Elijah Newren <redacted>
---
builtin/log.c | 23 +++++++++++++++++
diff-merges.c | 12 +++++++++
log-tree.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++
revision.h | 6 ++++-
4 files changed, 109 insertions(+), 1 deletion(-)
@@ -51,6 +53,7 @@ static int default_encode_email_headers = 1;staticintdecoration_style;staticintdecoration_given;staticintuse_mailmap_config=1;+staticstructtmp_objdir*tmp_objdir;staticconstchar*fmt_patch_subject_prefix="PATCH";staticintfmt_patch_name_max=FORMAT_PATCH_NAME_MAX_DEFAULT;staticconstchar*fmt_pretty;
@@ -407,6 +410,17 @@ static int cmd_log_walk(struct rev_info *rev)intsaved_nrl=0;intsaved_dcctc=0;+if(rev->remerge_diff){+tmp_objdir=tmp_objdir_create();+if(!tmp_objdir)+die(_("unable to create temporary object directory"));+tmp_objdir_make_primary(the_repository,tmp_objdir);++strbuf_init(&rev->remerge_objdir_location,0);+strbuf_addstr(&rev->remerge_objdir_location,+tmp_objdir_path(tmp_objdir));+}+if(rev->early_output)setup_early_output();
@@ -449,6 +463,13 @@ static int cmd_log_walk(struct rev_info *rev)rev->diffopt.no_free=0;diff_free(&rev->diffopt);+if(rev->remerge_diff){+strbuf_release(&rev->remerge_objdir_location);+tmp_objdir_remove_as_primary(the_repository,tmp_objdir);+tmp_objdir_destroy(tmp_objdir);+tmp_objdir=NULL;+}+if(rev->diffopt.output_format&DIFF_FORMAT_CHECKDIFF&&rev->diffopt.flags.check_failed){return02;
@@ -1943,6 +1964,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)die(_("--name-status does not make sense"));if(rev.diffopt.output_format&DIFF_FORMAT_CHECKDIFF)die(_("--check does not make sense"));+if(rev.remerge_diff)+die(_("--remerge_diff does not make sense"));if(!use_patch_format&&(!rev.diffopt.output_format||
@@ -902,6 +905,60 @@ static int do_diff_combined(struct rev_info *opt, struct commit *commit)return!opt->loginfo;}+staticintdo_remerge_diff(structrev_info*opt,+structcommit_list*parents,+structobject_id*oid,+structcommit*commit)+{+structmerge_optionso;+structcommit_list*bases;+structmerge_resultres;+structpretty_print_contextctx={0};+structstrbufcommit1=STRBUF_INIT;+structstrbufcommit2=STRBUF_INIT;++/* Setup merge options */+init_merge_options(&o,the_repository);+memset(&res,0,sizeof(res));+o.show_rename_progress=0;++ctx.abbrev=DEFAULT_ABBREV;+format_commit_message(parents->item,"%h (%s)",&commit1,&ctx);+format_commit_message(parents->next->item,"%h (%s)",&commit2,&ctx);+o.branch1=commit1.buf;+o.branch2=commit2.buf;+o.record_conflict_msgs_in_tree=1;++/* Parse the relevant commits and get the merge bases */+parse_commit_or_die(parents->item);+parse_commit_or_die(parents->next->item);+bases=get_merge_bases(parents->item,parents->next->item);++/* Re-merge the parents */+merge_incore_recursive(&o,+bases,parents->item,parents->next->item,+&res);++/* Show the diff */+diff_tree_oid(&res.tree->object.oid,oid,"",&opt->diffopt);+log_tree_diff_flush(opt);++/* Cleanup */+strbuf_release(&commit1);+strbuf_release(&commit2);+merge_finalize(&o,&res);++/* Clean up the temporary object directory */+if(opt->remerge_objdir_location.buf!=NULL&&+*opt->remerge_objdir_location.buf!='\0')+remove_dir_recursively(&opt->remerge_objdir_location,+REMOVE_DIR_KEEP_TOPLEVEL);+else+BUG("unable to remove temporary object directory");++return!opt->loginfo;+}+/**Showthediffofacommit.*
@@ -198,7 +198,8 @@ struct rev_info {combine_merges:1,combined_all_paths:1,dense_combined_merges:1,-first_parent_merges:1;+first_parent_merges:1,+remerge_diff:1;/* Format info */intshow_notes;
@@ -320,6 +321,9 @@ struct rev_info {/* misc. flags related to '--no-kept-objects' */unsignedkeep_pack_cache_flags;++/* Location where temporary objects for remerge-diff are written. */+structstrbufremerge_objdir_location;};intref_excluded(structstring_list*,constchar*path);
On 31/08/21 09.26, Elijah Newren via GitGitGadget wrote:
Here are some patches to add a --remerge-diff capability to show & log,
which works by comparing merge commits to an automatic remerge (note that
the automatic remerge tree can contain files with conflict markers).
Here are some example commits you can try this out on (with git show
--remerge-diff $COMMIT):
* git.git conflicted merge: 07601b5b36
* git.git non-conflicted change: bf04590ecd
* linux.git conflicted merge: eab3540562fb
* linux.git non-conflicted change: 223cea6a4f05
<snip>...
In regards to the performance point above, the timing for running the
following command:
time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l
in linux.git (with v5.4 checked out, since my copy of linux is very out of
date) is as follows:
DIFF_FLAG=--cc: 71m 31.536s
DIFF_FLAG=--remerge-diff: 31m 3.170s
Note that there are 62476 merges in this history. Also, output size is:
DIFF_FLAG=--cc: 2169111 lines
DIFF_FLAG=--remerge-diff: 2458020 lines
On Tue, Aug 31, 2021 at 4:05 AM Bagas Sanjaya [off-list ref] wrote:
On 31/08/21 09.26, Elijah Newren via GitGitGadget wrote:
quoted
Here are some patches to add a --remerge-diff capability to show & log,
which works by comparing merge commits to an automatic remerge (note that
the automatic remerge tree can contain files with conflict markers).
Here are some example commits you can try this out on (with git show
--remerge-diff $COMMIT):
* git.git conflicted merge: 07601b5b36
* git.git non-conflicted change: bf04590ecd
* linux.git conflicted merge: eab3540562fb
* linux.git non-conflicted change: 223cea6a4f05
<snip>...
quoted
In regards to the performance point above, the timing for running the
following command:
time git log --min-parents=2 --max-parents=2 $DIFF_FLAG | wc -l
in linux.git (with v5.4 checked out, since my copy of linux is very out of
date) is as follows:
DIFF_FLAG=--cc: 71m 31.536s
DIFF_FLAG=--remerge-diff: 31m 3.170s
Note that there are 62476 merges in this history. Also, output size is:
DIFF_FLAG=--cc: 2169111 lines
DIFF_FLAG=--remerge-diff: 2458020 lines
Not your fault & this pre-dates your patch, but FWIW I prefer our APIs
that don't have these "hidden struct" shenanigans (like say "struct
string_list") so a caller could just access this, we can just declare it
"const" appropriately.
We're also all in-tree friends here, so having various accessors for no
reason other than to access struct members seems a bit too much.
All of which is to say if you + Neeraj come up with some common API I
for one wouldn't mind moving the struct deceleration to tmp-objdir.h,
but it looks like in his version it can stay "private" more easily.
In this particular case I hadn't understood on a previous reading of
tmp-objdir.c why this "path" isn't a statically allocated "char
path[PATH_MAX]", or a least we that hardcoded "1024" should be a
PATH_MAX, as it is in some other cases.
But I digress :)
...but all of the "tmp_objdir" usage is in one function, can't the
variable be declared here instead?
We need to hand the "remerge_objdir_location" off to the "rev_info"
struct, but that seems separate from its lifetime.
Re my [1] & [2] I like Neeraj's "atexit cleanup" approach better,
perhaps that makes your cleanup in log-tree.c redundant or easier?
Per [2] it looks like you need to "hand off" the
"remerge_objdir_location", so having the struct live in tmp-objdir.h as
I suggested in [2] might make that work...
1. https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/87r1d9xh71.fsf@evledraar.gmail.com/
From: Jeff King <hidden> Date: 2021-09-28 22:29:20
On Tue, Aug 31, 2021 at 02:26:35AM +0000, Elijah Newren via GitGitGadget wrote:
There are several considerations here:
* We have to pick file(s) where we write these conflict messages too
* We want to make it as clear as possible that it's not a real file
but a set of messages about another file
* We want conflict messages about a file to appear near the file in
question in a diff, preferably immediately preceding the file in
question
* Related to the above, per-file conflict messages are preferred
over lumping all conflict messages into one big file
To achive the above:
* We put the conflict messages for $filename in
$filename[0:-1] + " " + $filename[-1] + ".conflict_msg"
or, in words, we insert a space before the final character of
the filename and then also add ".conflict_msg" at the end.
It took me a minute to understand the space thing. I thought at first it
was about avoiding conflicts with existing names (and while it might
help in practice, it's not a guarantee). But I think it's about the
"appear preceding the file" goal. The space sorts before any other
printable character in the final position.
That's...simultaneously clever and gross. My biggest complaint is that
the space looks like a bug in the output.
Using another character like "." might not be too bad, as it's also
fairly early in the ascii table. But it's really this "do it before the
last character" thing that is key to getting the ordering right.
Just brainstorming some alternatives:
- we have diff.orderFile, etc. Could we stuff this data into a less
confusing name (even just "$filename.conflict_msg"), and then provide
a custom ordering to the diff code? I think it could be done by
generating a static ordering ahead of time, but it might even just be
possible to tell diffcore_order() to take the ".conflict_msg"
extension into account in its comparison function.
- there can be other non-diff data between the individual segments. For
example, "patch" will skip over non-diff lines. And certainly in Git
we have our own custom headers. I'm wondering if we could attach
these annotations to the diff-pair somehow, and then show something
like:
diff --git a/foo.c b/foo.c
index 1234abcd..5678cdef 100644
conflict modify/delete foo.c
--- a/foo.c
+++ b/foo.c
@@ some actual diff starts here @@
Obviously such a thing can't really be applied. But then you wouldn't
want to apply the addition of "my.file e.conflict_msg" either.
I dunno. The latter especially is definitely more work, and requires a
bit more cooperation between the merge and diff code. In particular, you
can't just feed a straight tree to the diff anymore. We have to hold
back the annotations, and then apply them to the resulting diff. But I
think the output is much more pleasing to the eye.
-Peff
From: Jeff King <hidden> Date: 2021-09-28 22:38:02
On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted>
Whenever ll-merge encounters a binary file, it prints a warning to
stderr. However, for the --remerge-diff option we want to add, we need
to capture all conflict messages and show them in a diff instead of
dumping them straight to stdout or stderr. Add some new API that will
allow us to capture this output and display it in our preferred method.
This is a reasonable strategy for error-handling in general (though some
more thoughts below).
There's a lot of plumbing this variable through. This is probably too
gross, but another option would be to call set_warn_routine() to
override it temporarily. It's gross because it affects everything, not
just this call stack (and I also think we'd need to beef up the warn
routine code to handle some of the rough edges).
I do wonder if the ll_merge() code should avoid calling warning() in the
first place. It is after all, meant to be "low-level". We already return
an error code from the function. I wonder if returning a more detailed
code instead, like:
enum LL_MERGE_RESULT {
LL_MERGE_OK = 0,
LL_MERGE_CONFLICT,
LL_MERGE_BINARY_CONFLICT,
};
would let the caller do the sensible thing.
-Peff
From: Jeff King <hidden> Date: 2021-09-28 22:39:54
On Tue, Aug 31, 2021 at 02:26:37AM +0000, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted>
Instead of immediately printing ll-merge warnings to stderr, we save
them in our output strbuf. Besides allowing us to move these warnings
to a special file for --remerge-diff, this has two other benefits for
regular merges done by merge-ort:
* The deferral of messages ensures we can print all messages about
any given path together (merge-recursive was known to sometimes
intersperse messages about other paths, particularly when renames
were involved).
* The deferral of messages means we can avoid printing spurious
conflict messages when we just end up aborting due to local user
modifications in the way. (In contrast to merge-recursive.c which
prematurely checks for local modifications in the way via
unpack_trees() and gets the check wrong both in terms of false
positives and false negatives relative to renames, merge-ort does
not perform the local modifications in the way check until the
checkout() step after the full merge has been computed.)
Yeah, this is a good example of why having ll_merge() print warnings in
the first place is probably wrong. If you buy my argument that
ll_merge() should be returning an enum, then this code becomes IMHO even
nicer, as you can generate your own sensible message in the caller.
-Peff
From: Jeff King <hidden> Date: 2021-09-28 23:17:24
On Tue, Aug 31, 2021 at 02:26:38AM +0000, Elijah Newren via GitGitGadget wrote:
From: Elijah Newren <redacted>
The tmp_objdir API provides the ability to create temporary object
directories, but was designed with the goal of having subprocesses
access these object stores, followed by the main process migrating
objects from it to the main object store or just deleting it. The
subprocesses would view it as their primary datastore and write to it.
For the --remerge-diff option we want to add to show & log, we want all
writes of intermediate merge results (both blobs and trees) to go to
this alternate object store; since those writes will be done by the main
process, we need this "alternate" object store to actually be the
primary object store. When show & log are done, they'll simply remove
this temporary object store.
I think this is consistent with the original design of tmp_objdir. I
just never needed to do anything in-process before, so overriding the
environment of sub-processes was sufficient.
I do think these are dangerous and may cause bugs, though. Anything you
write while the tmp_objdir is marked as "primary" is going to go away
when you remove it. So any object names you reference outside of that,
either:
- by another object that you create after the tmp_objdir is removed;
or
- in a ref
are going to turn into repository corruption. Of course that's true for
the existing sub-processes, too, but here we're touching a wider variety
of code.
Obviously the objects we write as part of remerge-diff are meant to be
temporary, and we'll never reference them in any other way. And usually
we would not expect a diff to write any other objects. But one thing
that comes to mind if textconv caching.
If you do a remerge diff on a blob that uses textconv, and the caching
feature is turned on, then we'll write out a new blob with the cached
value, and eventually a new tree and refs/notes/ pointer referencing it.
I'm not sure of the timing of all of that, but it seems likely to me
that at least some of that will end up in your tmp_objdir.
If you remove the tmp_objdir as the primary as soon as you're done with
the merge, but before you run the diff, you might be OK, though.
If not, then I think the solution is probably not to install this as the
"primary", but rather:
- do the specific remerge-diff writes we want using a special "write
to this object dir" variant of write_object_file()
- install the tmp_objdir as an alternate, so the reading side (which
is diff code that doesn't otherwise know about our remerge-diff
trickery) can find them
And that lets other writers avoid writing into the temporary area
accidentally.
In that sense this is kind of like the pretend_object_file() interface,
except that it's storing the objects on disk instead of in memory. Of
course another way of doing that would be to stuff the object data into
tempfiles and just put pointers into the in-memory cached_objects array.
It's also not entirely foolproof (nor is the existing
pretend_object_file()). Any operation which is fooled into thinking we
have object X because it's in the fake-object list or the tmp_objdir may
reference that object erroneously, creating a corruption. But it's still
safer than allowing arbitrary writes into the tmp_objdir.
Side note: The pretend_object_file() approach is actually even better,
because we know the object is fake. So it does not confuse
write_object_file()'s "do we already have this object" freshening
check.
I suspect it could even be made faster than the tmp_objdir approach.
From our perspective, these objects really are tempfiles. So we could
write them as such, not worrying about things like fsyncing them,
naming them into place, etc. We could just write them out, then mmap
the results, and put the pointers into cached_objects (currently it
insists on malloc-ing a copy of the input buffer, but that seems like
an easy extension to add).
In fact, I think you could get away with just _one_ tempfile per
merge. Open up one tempfile. Write out all of the objects you want to
"store" into it in sequence, and record the lseek() offsets before and
after for each object. Then mmap the whole result, and stuff the
appropriate pointers (based on arithmetic with the offsets) into the
cached_objects list.
As a bonus, this tempfile can easily be in $TMPDIR, meaning
remerge-diff could work on a read-only repository (and the OS can
perhaps handle the data better, especially if $TMPDIR isn't a regular
filesystem).
We also need one more thing -- `git log --remerge-diff` can cause the
temporary object store to fill up with loose objects. Rather than
trying to gc that are known garbage anyway, we simply want to know the
location of the temporary object store so we can purge the loose objects
after each merge.
This paragraph confused me. At first I thought you were talking about
how to avoid calling "gc --auto" because we don't want to waste time
thinking all those loose objects were worth gc-ing. But we wouldn't do
that anyway (git-log does not expect to make objects so doesn't call it
at all, and anyway you'd expect it to happen at the end of a process,
after we've already removed the tmp_objdir).
But I think you just mean: we can build up a bunch of loose objects,
which is inefficient. We don't want to gc them, because that's even less
efficient. So we want to clean them out between merges.
But I don't see any code to do that here. I guess that's maybe why
you've added tmp_objdir_path(), to find them later. But IMHO this would
be much better encapsulated as tmp_objdir_clear_objects() or something.
But simpler still: is there any reason not to just drop and re-create
the tmp-objdir for each merge? It's not very expensive to do so (and
certainly not compared to the cost of actually writing out the objects).
-Peff
...but all of the "tmp_objdir" usage is in one function, can't the
variable be declared here instead?
That's a very good point.
We need to hand the "remerge_objdir_location" off to the "rev_info"
struct, but that seems separate from its lifetime.
Given Peff's suggestion elsewhere, though, to destroy the tmp_objdir
after each merge and create a new one, I wonder if I should actually
be passing a tmp_objdir** to rev_info (allowing log-tree to do the
work of destroying and creating a new one after each merge, instead of
using the "remerge_objdir_location" to run a recursive delete of
files). That'd still work with your idea to remove the statically
scoped variable, though.
Re my [1] & [2] I like Neeraj's "atexit cleanup" approach better,
perhaps that makes your cleanup in log-tree.c redundant or easier?
Having an atexit cleanup as a safety measure seems fine. However, I
don't like avoiding the manual cleanup step and relying on atexit
cleanup; I'd go so far as to say I think that'd be a bug, at least for
my usage. It presumes one-shot usage, whereas I'd rather move git to
being more library-like.
However, fully destroying the tmp_objdir probably makes the cleanup in
log-tree.c easier.
On Tue, Sep 28, 2021 at 3:37 PM Jeff King [off-list ref] wrote:
On Tue, Aug 31, 2021 at 02:26:36AM +0000, Elijah Newren via GitGitGadget wrote:
quoted
From: Elijah Newren <redacted>
Whenever ll-merge encounters a binary file, it prints a warning to
stderr. However, for the --remerge-diff option we want to add, we need
to capture all conflict messages and show them in a diff instead of
dumping them straight to stdout or stderr. Add some new API that will
allow us to capture this output and display it in our preferred method.
This is a reasonable strategy for error-handling in general (though some
more thoughts below).
There's a lot of plumbing this variable through. This is probably too
gross, but another option would be to call set_warn_routine() to
override it temporarily. It's gross because it affects everything, not
just this call stack (and I also think we'd need to beef up the warn
routine code to handle some of the rough edges).
I do wonder if the ll_merge() code should avoid calling warning() in the
first place. It is after all, meant to be "low-level". We already return
an error code from the function. I wonder if returning a more detailed
code instead, like:
enum LL_MERGE_RESULT {
LL_MERGE_OK = 0,
LL_MERGE_CONFLICT,
LL_MERGE_BINARY_CONFLICT,
};
would let the caller do the sensible thing.
Ooh, I like this idea. I'll have to change all the callers, but
there's only about half a dozen or so...
Not your fault & this pre-dates your patch, but FWIW I prefer our APIs
that don't have these "hidden struct" shenanigans (like say "struct
string_list") so a caller could just access this, we can just declare it
"const" appropriately.
We're also all in-tree friends here, so having various accessors for no
reason other than to access struct members seems a bit too much.
That's a fair point, but just to play counterpoint for a minute...
FWIW, I dislike when our public facing APIs are polluted with all
kinds of internal details. merge-recursive being a case in point.
When writing merge-ort, although I had a struct with public fields,
that struct also contained an opaque struct (pointer) within it to
hide several private fields. (I would have liked to hide or remove a
few more fields, but couldn't do so while the merge_recursive_options
struct was shared between both merge-ort and merge-recursive.)
That said, I agree it can certainly be overdone, and tmp_objdir is
pretty simple. However, sometimes even in simple cases I like when
folks make use of an opaque struct because it means folks put some
effort into thinking more about the API that should be exposed.
That's something we as a project have often overlooked in the past, as
evidenced both by our one-shot usage mentality, and the existence of
external projects like libgit2 attempting to correct this design
shortcoming. I'd like git to move more towards being structured as a
reusable library as well as a useful command-line tool.
All of which is to say if you + Neeraj come up with some common API I
for one wouldn't mind moving the struct deceleration to tmp-objdir.h,
but it looks like in his version it can stay "private" more easily.
Peff's ideas to just delete and recreate the temporary object store
after every merge would let me reduce the API extensions and keep
things more private. So I think our ideas are converging a bit. :-)
In this particular case I hadn't understood on a previous reading of
tmp-objdir.c why this "path" isn't a statically allocated "char
path[PATH_MAX]", or a least we that hardcoded "1024" should be a
PATH_MAX, as it is in some other cases.
On Tue, Sep 28, 2021 at 4:17 PM Jeff King [off-list ref] wrote:
On Tue, Aug 31, 2021 at 02:26:38AM +0000, Elijah Newren via GitGitGadget wrote:
quoted
From: Elijah Newren <redacted>
The tmp_objdir API provides the ability to create temporary object
directories, but was designed with the goal of having subprocesses
access these object stores, followed by the main process migrating
objects from it to the main object store or just deleting it. The
subprocesses would view it as their primary datastore and write to it.
For the --remerge-diff option we want to add to show & log, we want all
writes of intermediate merge results (both blobs and trees) to go to
this alternate object store; since those writes will be done by the main
process, we need this "alternate" object store to actually be the
primary object store. When show & log are done, they'll simply remove
this temporary object store.
I think this is consistent with the original design of tmp_objdir. I
just never needed to do anything in-process before, so overriding the
environment of sub-processes was sufficient.
I do think these are dangerous and may cause bugs, though. Anything you
write while the tmp_objdir is marked as "primary" is going to go away
when you remove it. So any object names you reference outside of that,
either:
- by another object that you create after the tmp_objdir is removed;
or
- in a ref
are going to turn into repository corruption. Of course that's true for
the existing sub-processes, too, but here we're touching a wider variety
of code.
Obviously the objects we write as part of remerge-diff are meant to be
temporary, and we'll never reference them in any other way. And usually
we would not expect a diff to write any other objects. But one thing
that comes to mind if textconv caching.
If you do a remerge diff on a blob that uses textconv, and the caching
feature is turned on, then we'll write out a new blob with the cached
value, and eventually a new tree and refs/notes/ pointer referencing it.
I'm not sure of the timing of all of that, but it seems likely to me
that at least some of that will end up in your tmp_objdir.
Interesting. Thanks for explaining this case, and for the testcase in
the other thread.
So there are other parts of running `git log` that are not read-only,
besides what I'm doing. That's unfortunate. Can we just disable
those things in this section of the code?
Also, I think it makes sense that textconv caching causes a problem
via writing new blobs and trees *and* refs that reference these blobs
and trees. However, I'm not sure I'm really grasping it, because I
don't see how your solutions are safe either. I'll mention more after
each one.
If you remove the tmp_objdir as the primary as soon as you're done with
the merge, but before you run the diff, you might be OK, though.
It has to be after I run the diff, because the diff needs access to
the temporary files to diff against them.
If not, then I think the solution is probably not to install this as the
"primary", but rather:
- do the specific remerge-diff writes we want using a special "write
to this object dir" variant of write_object_file()
- install the tmp_objdir as an alternate, so the reading side (which
is diff code that doesn't otherwise know about our remerge-diff
trickery) can find them
And that lets other writers avoid writing into the temporary area
accidentally.
Doesn't this have the same problem? If something similar to textconv
caching were to create new trees that referenced the existing blobs
and then added a ref that referred to that tree, then you have the
exact same problem, right? The new tree and the new ref would be
corrupt as soon as the tmp-objdir went away. Whether or not other
callers write to the temporary area isn't the issue, it's whether they
write refs that can refer to anything from the temporary area. Or am
I missing something?
In that sense this is kind of like the pretend_object_file() interface,
except that it's storing the objects on disk instead of in memory. Of
course another way of doing that would be to stuff the object data into
tempfiles and just put pointers into the in-memory cached_objects array.
It's also not entirely foolproof (nor is the existing
pretend_object_file()). Any operation which is fooled into thinking we
have object X because it's in the fake-object list or the tmp_objdir may
reference that object erroneously, creating a corruption. But it's still
safer than allowing arbitrary writes into the tmp_objdir.
Why is the pretend_object_file() interface safer? Does it disable the
textconv caching somehow? I don't see why it helps avoid this
problem.
Side note: The pretend_object_file() approach is actually even better,
because we know the object is fake. So it does not confuse
write_object_file()'s "do we already have this object" freshening
check.
Oh, that's interesting. That would be helpful.
I suspect it could even be made faster than the tmp_objdir approach.
From our perspective, these objects really are tempfiles. So we could
write them as such, not worrying about things like fsyncing them,
naming them into place, etc. We could just write them out, then mmap
the results, and put the pointers into cached_objects (currently it
insists on malloc-ing a copy of the input buffer, but that seems like
an easy extension to add).
In fact, I think you could get away with just _one_ tempfile per
merge. Open up one tempfile. Write out all of the objects you want to
"store" into it in sequence, and record the lseek() offsets before and
after for each object. Then mmap the whole result, and stuff the
appropriate pointers (based on arithmetic with the offsets) into the
cached_objects list.
As a bonus, this tempfile can easily be in $TMPDIR, meaning
remerge-diff could work on a read-only repository (and the OS can
perhaps handle the data better, especially if $TMPDIR isn't a regular
filesystem).
Interesting. I'm not familiar with the pretend_object_file() code,
but I think I catch what you're saying. I don't see anything in
object-file.c that allows removing items from the cache, as I would
want to do, but I'm sure I could add something.
(I'm still not sure how this avoids the problem of things like
textconv caching trying to write an object that references one of
these, though. Should we just manually disable textconv caching
during a remerge-diff?)
quoted
We also need one more thing -- `git log --remerge-diff` can cause the
temporary object store to fill up with loose objects. Rather than
trying to gc that are known garbage anyway, we simply want to know the
location of the temporary object store so we can purge the loose objects
after each merge.
This paragraph confused me. At first I thought you were talking about
how to avoid calling "gc --auto" because we don't want to waste time
thinking all those loose objects were worth gc-ing. But we wouldn't do
that anyway (git-log does not expect to make objects so doesn't call it
at all, and anyway you'd expect it to happen at the end of a process,
after we've already removed the tmp_objdir).
But I think you just mean: we can build up a bunch of loose objects,
which is inefficient. We don't want to gc them, because that's even less
efficient. So we want to clean them out between merges.
Yes, sorry that wasn't clear.
But I don't see any code to do that here. I guess that's maybe why
you've added tmp_objdir_path(), to find them later. But IMHO this would
be much better encapsulated as tmp_objdir_clear_objects() or something.
But simpler still: is there any reason not to just drop and re-create
the tmp-objdir for each merge? It's not very expensive to do so (and
certainly not compared to the cost of actually writing out the objects).
Ooh, indeed. I like that, and it turns out that the majority of the
cost of dropping the tmp_objdir is the recursive directory removal,
that I wanted anyway as part of me inbetween-merges cleanup.
But, of course, if we go the pretend_object_file() route then the
tmp_objdir() stuff probably becomes moot.
On Tue, Sep 28, 2021 at 3:29 PM Jeff King [off-list ref] wrote:
On Tue, Aug 31, 2021 at 02:26:35AM +0000, Elijah Newren via GitGitGadget wrote:
quoted
There are several considerations here:
* We have to pick file(s) where we write these conflict messages too
* We want to make it as clear as possible that it's not a real file
but a set of messages about another file
* We want conflict messages about a file to appear near the file in
question in a diff, preferably immediately preceding the file in
question
* Related to the above, per-file conflict messages are preferred
over lumping all conflict messages into one big file
To achive the above:
* We put the conflict messages for $filename in
$filename[0:-1] + " " + $filename[-1] + ".conflict_msg"
or, in words, we insert a space before the final character of
the filename and then also add ".conflict_msg" at the end.
It took me a minute to understand the space thing. I thought at first it
was about avoiding conflicts with existing names (and while it might
help in practice, it's not a guarantee). But I think it's about the
"appear preceding the file" goal. The space sorts before any other
printable character in the final position.
Yeah, it's all about the ordering. I guess it helps slightly with
conflict avoidance, but I cannot rely on it; I have to check for
colliding files and potentially tweak the filename further.
That's...simultaneously clever and gross. My biggest complaint is that
the space looks like a bug in the output.
Using another character like "." might not be too bad, as it's also
fairly early in the ascii table. But it's really this "do it before the
last character" thing that is key to getting the ordering right.
Just brainstorming some alternatives:
- we have diff.orderFile, etc. Could we stuff this data into a less
confusing name (even just "$filename.conflict_msg"), and then provide
a custom ordering to the diff code? I think it could be done by
generating a static ordering ahead of time, but it might even just be
possible to tell diffcore_order() to take the ".conflict_msg"
extension into account in its comparison function.
I can't just go on the ".conflict_msg" extension. As you noted above,
this scheme is not sufficient for avoiding collisions. So I need to
append extra "cruft" to the name in the case of collisions -- meaning
we can't special case on just that extension.
I also don't like how diff.orderFile provides a global ordering of the
files listed, rather than providing some scheme for relative
orderings. That'd either force me to precompute the diff to determine
all the files that were different so I can list _all_ of them, or put
up with the fact that the files with non-content conflicts will be
listed very first in the output, even if their name is
'zee-last-file.c' -- surprising users at the output ordering.
This also means that if the user had their own ordering defined, then
I'm overriding it and messing up their ordering, which might be
problematic.
So, I'm not so sure about this solution; it feels like it introduces
bigger holes than the ugly space character it is fixing.
- there can be other non-diff data between the individual segments. For
example, "patch" will skip over non-diff lines. And certainly in Git
we have our own custom headers. I'm wondering if we could attach
these annotations to the diff-pair somehow, and then show something
like:
diff --git a/foo.c b/foo.c
index 1234abcd..5678cdef 100644
conflict modify/delete foo.c
A couple things here...
First, I'm not so sure I like the abbreviation here. Just knowing
"modify/delete" might be enough in some cases, but I'd rather have the
full messages that would have been printed to the console, e.g.:
CONFLICT (modify/delete): foo.c deleted in HASH1 (SHORT
SUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2
(SHORT SUMMARY2) of foo.c left in tree.
because I think the commit references are useful context. That extra
context might be of little use for many modify/delete conflicts, but
is much more important for conflicts involving renames; e.g.
"rename/rename" is much less useful than being able to know the
original name of the file and with which parent commit each filename
is associated. So, that raises the question: could we pack all that
information from the full conflict notice into these conflict
header(s)? (And do we have to special case the code to print it all
on one line when doing the remerge-diff since the diff output needs
them to be one-line headers?)
Second, what about when there are multiple non-content conflict types
for a single file, e.g. rename/delete + rename/add + modify/delete +
mode conflict + unmergeable binary? (Yes, I think it's possible for
one path to have all five of those: (1) source file deleted on one
side, renamed on the other, (2) rename target on one side matches new
file added on other side of history, (3) renamed file also had its
contents modified, thus modify vs. delete, (4) added file on other
side of history had a different mode, (5) added file on other side of
history is a binary.) Do we just use multiple conflict headers?
Third, what about the cases where there is no diff, just conflict
headers? (I suspect many modify/delete or rename/delete or binary
files may end up in such a situation.)
I don't think any of those are deal breakers, but it means more work,
and maybe also other forms of ugliness.
--- a/foo.c
+++ b/foo.c
@@ some actual diff starts here @@
Obviously such a thing can't really be applied. But then you wouldn't
want to apply the addition of "my.file e.conflict_msg" either.
Nit: "my.fil e.conflict_msg", not "my.file e.conflict_msg" (the 'e' in
'file' is not repeated, otherwise the auxiliary file wouldn't sort
before its companion file)
I dunno. The latter especially is definitely more work, and requires a
bit more cooperation between the merge and diff code. In particular, you
can't just feed a straight tree to the diff anymore. We have to hold
back the annotations, and then apply them to the resulting diff. But I
think the output is much more pleasing to the eye.
It's certainly an interesting idea. It's a lot more work, it involves
the inability to feed a straight tree to a diff would require piping
things through several different layers (merge -> log -> diff, and
possible multiple diff layers), it may mean we need special handling
for when there are only conflict headers for a file with no file
differences, the length of the conflict headers could be comically
long, and it's all essentially for what is a rather uncommon case
anyway. But, on the plus side, it does avoid the rather ugly space.
I'll have to think about it.
From: Jeff King <hidden> Date: 2021-09-30 07:26:49
On Tue, Sep 28, 2021 at 10:05:31PM -0700, Elijah Newren wrote:
So there are other parts of running `git log` that are not read-only,
besides what I'm doing. That's unfortunate. Can we just disable
those things in this section of the code?
It might be possible for the cache (we'd still generate entries, but
just not write them). It's "just" an optimization, though it's possible
there are cases where it's really annoying to do so. I'd imagine those
are a stretch. I do use textconv to decrypt gpg blobs with a key on a
hardware token, for example. I don't require a touch for each one,
though, nor would I use textconv caching anyway (since the point is not
to store the unencrypted contents).
It wouldn't necessarily be OK to disable object writes in general.
Though I'd think since a diff is _conceptually_ a read-only operation
that any writes it wants to do would be in the same caching /
optimization boat. I'd also worry about the maintenance cost of having
to annotate any such case.
Also, I think it makes sense that textconv caching causes a problem
via writing new blobs and trees *and* refs that reference these blobs
and trees. However, I'm not sure I'm really grasping it, because I
don't see how your solutions are safe either. I'll mention more after
each one.
Heh. So I originally wrote more on this in my earlier email, but worried
I was getting too into the weeds and would confuse you, so I pared it
down. But I managed to be confusing anyway. :)
Here's one mental model. If you have a temporary object store, then
these are the bad things that can happen:
1. Some code may write an object to it, thinking the data is saved,
but we later throw it away.
2. Some code may _skip_ writing an object, thinking that we already
have that object available (when in fact we'll later throw it
away).
3. Some code may record the fact that we have an object in memory, and
then later (after the temporary object store is not in use), may
point to it when writing an object or reference.
Pointing the primary object-dir at a temporary directory (like your
patch does) means we're subject to all three.
Problem (1) happens because unrelated code may not realize we've swapped
out the primary object writing code for this throw-away directory. But
we could require writers to explicitly indicate that they want to write
to the temporary area, while allowing other writes to go to the regular
object store. This could be done with a flag or variant of
write_object_file(). Or if the temporary area isn't a regular object
directory at all (like the whole tempfile / pretend thing), then this
happens automatically, because the regular object-write paths don't even
know about our temporary area.
Problem (2) is mostly handled inside write_object_file(). It will
optimize out writes of objects we already have. So it needs to know when
we have an object "for real", and when we just have it in our temporary
store. If we make the tmp_objdir the primary, then it doesn't know this
(though we could teach it to check. In the pretend_object_file() system,
it already knows this (it uses the "freshen" code paths which actually
want to find the entry in the filesystem).
Problem (3) is the hardest one, because we don't really distinguish
between readers and writers. So if I call has_object_file(), is it
because I want to access the object? Or is it because I want to generate
a new reference to it, which must ensure that it exists? One of those is
OK to look at the tmp_objdir (and indeed, is the whole point of making
it in the first place), and the other is the first step to corrupting
the repository. ;)
With the environment variables we set up for running external commands
(like hooks) in the receive-pack quarantine, one bit of safety we have
is to prevent ref writes entirely in those commands. That works
reasonably well. Even if they may write new objects that might get
thrown away (if the push is rejected), they'd ultimately need to point
to those objects with a ref. And because those hooks spend their
_entire_ run-time in the quarantine state, they can't ever write a ref.
Whereas doing it in-process is a little dicier. Right now the textconv
cache writes out a new tree for every entry we add, so it happens to do
the ref write while the tmp-objdir is still in place. But as this
comment notes:
$ git grep -B6 notes_cache_write diff.c
diff.c- /*
diff.c- * we could save up changes and flush them all at the end,
diff.c- * but we would need an extra call after all diffing is done.
diff.c- * Since generating a cache entry is the slow path anyway,
diff.c- * this extra overhead probably isn't a big deal.
diff.c- */
diff.c: notes_cache_write(textconv->cache);
this is slow, and it would be perfectly reasonable to flush the cache at
the end of the process (e.g., using an atexit() handler). In which case
we'd hit this problem (3) exactly: we'd generate and remember objects
during the tmp-objdir period, but only later actually reference them.
This is more likely than the receive-pack hook case, because we're doing
it all in process.
quoted
If you remove the tmp_objdir as the primary as soon as you're done with
the merge, but before you run the diff, you might be OK, though.
It has to be after I run the diff, because the diff needs access to
the temporary files to diff against them.
Right, of course. I was too fixated on the object-write part, forgetting
that the whole point of the exercise is to later read them back. :)
quoted
If not, then I think the solution is probably not to install this as the
"primary", but rather:
- do the specific remerge-diff writes we want using a special "write
to this object dir" variant of write_object_file()
- install the tmp_objdir as an alternate, so the reading side (which
is diff code that doesn't otherwise know about our remerge-diff
trickery) can find them
And that lets other writers avoid writing into the temporary area
accidentally.
Doesn't this have the same problem? If something similar to textconv
caching were to create new trees that referenced the existing blobs
and then added a ref that referred to that tree, then you have the
exact same problem, right? The new tree and the new ref would be
corrupt as soon as the tmp-objdir went away. Whether or not other
callers write to the temporary area isn't the issue, it's whether they
write refs that can refer to anything from the temporary area. Or am
I missing something?
The key thing here is in the first step, where remerge-diff is
explicitly saying "I want to write to this temporary object-dir". But
the textconv-cache code does not; it gets to write to the normal spot.
So it avoids problem (1).
You're right that it does not avoid problem (3) exactly. But triggering
that would require some code not just writing other objects or
references while the tmp-objdir is in place, but specifically
referencing the objects that remerge-diff did put into the tmp-objdir.
That seems a lot less likely to me (because the thing we're most worried
about is unrelated code that just happens to write while the tmp-objdir
is in place).
quoted
In that sense this is kind of like the pretend_object_file() interface,
except that it's storing the objects on disk instead of in memory. Of
course another way of doing that would be to stuff the object data into
tempfiles and just put pointers into the in-memory cached_objects array.
It's also not entirely foolproof (nor is the existing
pretend_object_file()). Any operation which is fooled into thinking we
have object X because it's in the fake-object list or the tmp_objdir may
reference that object erroneously, creating a corruption. But it's still
safer than allowing arbitrary writes into the tmp_objdir.
Why is the pretend_object_file() interface safer? Does it disable the
textconv caching somehow? I don't see why it helps avoid this
problem.
So hopefully it's clearer now from what I wrote above, but just
connecting the dots:
1. Unrelated code calling write_object_file() would still write real,
durable objects, as usual.
2. The write_object_file() "skip this write" optimization already
ignores the pretend_object_file() objects while checking "do we
have this object".
Interesting. I'm not familiar with the pretend_object_file() code,
but I think I catch what you're saying. I don't see anything in
object-file.c that allows removing items from the cache, as I would
want to do, but I'm sure I could add something.
Right, it's pretty limited now, and there's only one caller. And in fact
I've wanted to get rid of it, exactly because it can create the danger
we're discussing here. But I still think it's _less_ dangerous than
fully replacing the object directory.
(I'm still not sure how this avoids the problem of things like
textconv caching trying to write an object that references one of
these, though. Should we just manually disable textconv caching
during a remerge-diff?)
It can't avoid it completely, but in general, the textconv cache should
be writing and referencing its own objects. Even if they happen to be
the same as something remerge-diff generated, it won't know that until
it has tried to write it (and so that's why having write_object_file()
continue to really write the object is important).
Hopefully that clears up my line of thinking. I do think a lot of this
is kind of theoretical, in the sense that:
- textconv caching is an obscure feature that we _could_ just disable
during remerge-diffs
- there's a reasonable chance that there isn't any other code that
wants to write during a diff
But this is all scary and error-prone enough that I'd prefer an approach
that has the "least surprise". So any solution where random code calling
write_object_file() _can't_ accidentally write to a throw-away directory
seems like a safer less surprising thing.
It does mean that the remerge-diff code needs to be explicit in its
object writes to say "this needs to go to the temporary area" (whether
it's a flag, or calling into a totally different function or subsystem).
I'm hoping that's not hard to do (because its writes are done explicitly
by the remerge-diff code), but I worry that it may be (because you are
relying on more generic tree code to the write under the hood).
-Peff
From: Jeff King <hidden> Date: 2021-09-30 07:41:37
On Tue, Sep 28, 2021 at 09:22:32PM -0700, Elijah Newren wrote:
quoted
Not your fault & this pre-dates your patch, but FWIW I prefer our APIs
that don't have these "hidden struct" shenanigans (like say "struct
string_list") so a caller could just access this, we can just declare it
"const" appropriately.
We're also all in-tree friends here, so having various accessors for no
reason other than to access struct members seems a bit too much.
That's a fair point, but just to play counterpoint for a minute...
FWIW, I dislike when our public facing APIs are polluted with all
kinds of internal details. merge-recursive being a case in point.
When writing merge-ort, although I had a struct with public fields,
that struct also contained an opaque struct (pointer) within it to
hide several private fields. (I would have liked to hide or remove a
few more fields, but couldn't do so while the merge_recursive_options
struct was shared between both merge-ort and merge-recursive.)
That said, I agree it can certainly be overdone, and tmp_objdir is
pretty simple. However, sometimes even in simple cases I like when
folks make use of an opaque struct because it means folks put some
effort into thinking more about the API that should be exposed.
That's something we as a project have often overlooked in the past, as
evidenced both by our one-shot usage mentality, and the existence of
external projects like libgit2 attempting to correct this design
shortcoming. I'd like git to move more towards being structured as a
reusable library as well as a useful command-line tool.
Right, it was definitely a conscious decision to keep the tmp-objdir API
as slim as possible, just because it's such a complicated and confusing
thing in the first place.
For something like a strbuf, giving direct access to the fields makes
sense. Exposing the details of how the struct works (like accessing
".buf" as a NUL-terminated string) are part of its usefulness.
But tmp_objdir represents a more abstract concept, and I wanted to
insulate callers from the details.
That said, the notion of "this is the path of the objdir" is not that
contentious, so I don't mind it too much (but it would be a jump to
exposing the details at all).
-Peff
From: Jeff King <hidden> Date: 2021-09-30 07:46:45
On Thu, Sep 30, 2021 at 03:26:42AM -0400, Jeff King wrote:
quoted
quoted
If you remove the tmp_objdir as the primary as soon as you're done with
the merge, but before you run the diff, you might be OK, though.
It has to be after I run the diff, because the diff needs access to
the temporary files to diff against them.
Right, of course. I was too fixated on the object-write part, forgetting
that the whole point of the exercise is to later read them back. :)
Ah, no, I remember what I was trying to say here. The distinction is
between "remove the tmp_objdir" and "remove it as the primary".
I.e., if you do this:
1. create tmp_objdir
2. make tmp_objdir primary for writes
3. run the "merge" half of remerge-diff, writing objects into the
temporary space
4. stop having tmp_objdir as the primary; instead make it an alternate
5. run the diff
6. remove tmp_objdir totally
Then step 5 can't accidentally write objects into the temporary space,
but it can still read them. So it's not entirely safe, but it's safer,
and it would be a much smaller change.
Some ways it could go wrong:
- is it possible for the merge code to ever write an object? I kind of
wonder if we'd ever do any cache-able transformations as part of a
content-level merge. I don't think we do now, though.
- in step 5, write_object_file() may still be confused by the presence
of the to-be-thrown-away objects in the alternate. This is pretty
unlikely, as it implies that the remerge-diff wrote a blob or tree
that is byte-identical to something that the diff wants to write.
-Peff
From: Jeff King <hidden> Date: 2021-09-30 07:58:58
On Tue, Sep 28, 2021 at 11:25:20PM -0700, Elijah Newren wrote:
quoted
Just brainstorming some alternatives:
- we have diff.orderFile, etc. Could we stuff this data into a less
confusing name (even just "$filename.conflict_msg"), and then provide
a custom ordering to the diff code? I think it could be done by
generating a static ordering ahead of time, but it might even just be
possible to tell diffcore_order() to take the ".conflict_msg"
extension into account in its comparison function.
I can't just go on the ".conflict_msg" extension. As you noted above,
this scheme is not sufficient for avoiding collisions. So I need to
append extra "cruft" to the name in the case of collisions -- meaning
we can't special case on just that extension.
Sure, but we can call it filename.conflict_msg.1, etc, and the sort code
can pattern-match. It can never be fully robust (if you really did have
a foo.conflict_msg, we'd sort it differently), but I think it's OK if
the worst-case is that pathological trees get ordered slightly
sub-optimally).
That said, I think it could also make sense to annotate the conflict
files by giving them an unusual set of mode bits. That would be easier
to recognize (and while real trees _could_ have silly modes, we do
complain about them in fsck, so it shouldn't happen in practice).
I also don't like how diff.orderFile provides a global ordering of the
files listed, rather than providing some scheme for relative
orderings. That'd either force me to precompute the diff to determine
all the files that were different so I can list _all_ of them, or put
up with the fact that the files with non-content conflicts will be
listed very first in the output, even if their name is
'zee-last-file.c' -- surprising users at the output ordering.
This also means that if the user had their own ordering defined, then
I'm overriding it and messing up their ordering, which might be
problematic.
Agreed. I do think it may be too horrible unless you teach
diffcore_order() to actually understand your annotations in code. But
that wouldn't be too hard if it's done in the mode bits.
But I do think anything that avoids these pseudo-files is going to be a
lot cleaner overall.
quoted
- there can be other non-diff data between the individual segments. For
example, "patch" will skip over non-diff lines. And certainly in Git
we have our own custom headers. I'm wondering if we could attach
these annotations to the diff-pair somehow, and then show something
like:
diff --git a/foo.c b/foo.c
index 1234abcd..5678cdef 100644
conflict modify/delete foo.c
A couple things here...
I think Junio already indicated that we can pretty much make this look
like whatever we want. As soon as any reader sees "conflict", it should
happily ignore the rest as something it doesn't know about. And my short
example here was just meant to be illustrative. I agree it probably
needs more details (and the whole CONFLICT line that usually goes to
stderr is probably the best thing).
-Peff
[...]
That said, I think it could also make sense to annotate the conflict
files by giving them an unusual set of mode bits. That would be easier
to recognize (and while real trees _could_ have silly modes, we do
complain about them in fsck, so it shouldn't happen in practice).
Not your fault & this pre-dates your patch, but FWIW I prefer our APIs
that don't have these "hidden struct" shenanigans (like say "struct
string_list") so a caller could just access this, we can just declare it
"const" appropriately.
We're also all in-tree friends here, so having various accessors for no
reason other than to access struct members seems a bit too much.
That's a fair point, but just to play counterpoint for a minute...
FWIW, I dislike when our public facing APIs are polluted with all
kinds of internal details. merge-recursive being a case in point.
When writing merge-ort, although I had a struct with public fields,
that struct also contained an opaque struct (pointer) within it to
hide several private fields. (I would have liked to hide or remove a
few more fields, but couldn't do so while the merge_recursive_options
struct was shared between both merge-ort and merge-recursive.)
That said, I agree it can certainly be overdone, and tmp_objdir is
pretty simple. However, sometimes even in simple cases I like when
folks make use of an opaque struct because it means folks put some
effort into thinking more about the API that should be exposed.
That's something we as a project have often overlooked in the past, as
evidenced both by our one-shot usage mentality, and the existence of
external projects like libgit2 attempting to correct this design
shortcoming. I'd like git to move more towards being structured as a
reusable library as well as a useful command-line tool.
[This is somewhat of a continuation of
https://lore.kernel.org/git/87lf3etaih.fsf@evledraar.gmail.com/].
I like our APIs being usable, but right now we're not promising any
maintenance of APIs that work the same as git v2.30.0 or whatever.
I think some external users directly link to libgit.a, but they're
tracking whatever churn we have from release to release, just like
someone maintaining out-of-tree linux kernel drivers.
For maintenance of git.git itself I think it's going to stay like that
for any foreseeable future[1].
And that's before we even get into the practicalities of git's license
likely not winning over many (if any) libgit2 users. I think those users
are if anything migrating from libgit2 to git's plumbing CLI, or in some
cases folding what they needed into git.git itself (or just staying with
libgit2).
So assuming all of that I really don't see the point in general of
having a "foo" field that has the equivalent of get_foo() and set_foo()
accessors in the context of git.git.
That's something that's really useful if you're maintaining API and ABI
compatibility, but for us it's usually just verbosity. All the users are
in-tree, so just add "const" as appropriate, or perhaps we should give
the field a "priv" name or whatever.
Obviously not everything should be done that way, e.g. being able to
have trace2 start/end points for something you'd otherwise push/clear
from a "struct string_list" or whatever directly *is* usable, so it's
not black and white.
I'm just saying thaht using convoluted patterns *just* to get in the way
of some hypothetical code that's going to mess with your internals can
make things harder in other areas. E.g. the init/release pattern via
macros I mentioned in the linked E-Mail above.
[...]
quoted
In this particular case I hadn't understood on a previous reading of
tmp-objdir.c why this "path" isn't a statically allocated "char
path[PATH_MAX]", or a least we that hardcoded "1024" should be a
PATH_MAX, as it is in some other cases.
Thanks. I didn't know about that. I've fixed (some of that) up in some
semi-related WIP work I've got.
1. Aside: I *do* think it would be really useful for us to expose a
stable C API eventually, using some of the same license
workarounds/shenanigans (depending on your view) as the Linux kernel
did. I.e. to say that "we're GPLv2, but this small API is the
equivalent of our syscall interface".
Rather than that API being something that would look anything like
internal git.git users it would basically be a run_command() + Perl's
unpack/pack + output formatter on steroids.
I.e. we'd be able to invoke say a 'git cat-file --batch' in the same
process as the caller by essentially calling cmd_cat_file()
directly. Just with some callback replacements for "provide this on
stdin" and "this line accepts stdout" without actually involving
different processes or I/O.
You'd then have the equivalent of a --format output that would just
so happen to be in your OS's / C ABI's understood struct
format. Similar to "perldoc -f pack".
The maintenance burden of such an API would be trivial. It would
basically be a handful of functions just to setup input/output
callbacks.
Most of the interface would be a rather convoluted construction of
command-line parameters (like our own run_command() use), not ideal,
but most users are after structured data and eliminating process
overhead, and they'd get that.
We would need to replace fprintf() and the like with some structured
formatting mechanism, but that would be useful for non-API users as
new commands learning a shiny new --format parameter. You could even
use such a --format to have a non-C-API user get the same struct
output, perhaps for direct mapping into memory, say if you didn't
trust the built-in/command in question to not leak memory yet.
I don't know if this has been suggested before, just my 0.02. It's
also partially why I'm interested in making even-built-ins not leak
memory with some of my parallel SANITIZE=leak work.
Once you've got that you can usually call the cmd_whatever() in a
loop without much effort, and once you've got that, well, see above
:)
On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
quoted hunk
@@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' ' printf "\0\0" >binary-file && git add binary-file && git commit -m binary2 &&- test_must_fail git merge F >merge.out 2>merge.err &&- grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort+ then+ test_must_fail git merge F >merge.out &&+ grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out+ else+ test_must_fail git merge F >merge.out 2>merge.err &&+ grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err+ fi '
To save readers from eyeballing if a single character has changed here,
which I don't think it has, just do:
if ...
then
cmd >actual
else
other cmd >actual
fi &&
grep [...]
I.e. no need to duplicate the "grep" here just because of merge.out v.s. merge.err.
[...]
- test_must_fail git merge bin-main 2>stderr &&
- grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+ then
+ test_must_fail git merge bin-main >stdout &&
+ grep -i "warning.*cannot merge.*HEAD vs. bin-main" stdout
+ else
+ test_must_fail git merge bin-main 2>stderr &&
+ grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+ fi
On Thu, Sep 30, 2021 at 03:26:42AM -0400, Jeff King wrote:
On Tue, Sep 28, 2021 at 10:05:31PM -0700, Elijah Newren wrote:
quoted
So there are other parts of running `git log` that are not read-only,
besides what I'm doing. That's unfortunate. Can we just disable
those things in this section of the code?
It might be possible for the cache (we'd still generate entries, but
just not write them). It's "just" an optimization, though it's possible
there are cases where it's really annoying to do so. I'd imagine those
are a stretch. I do use textconv to decrypt gpg blobs with a key on a
hardware token, for example. I don't require a touch for each one,
though, nor would I use textconv caching anyway (since the point is not
to store the unencrypted contents).
It wouldn't necessarily be OK to disable object writes in general.
Though I'd think since a diff is _conceptually_ a read-only operation
that any writes it wants to do would be in the same caching /
optimization boat. I'd also worry about the maintenance cost of having
to annotate any such case.
quoted
Also, I think it makes sense that textconv caching causes a problem
via writing new blobs and trees *and* refs that reference these blobs
and trees. However, I'm not sure I'm really grasping it, because I
don't see how your solutions are safe either. I'll mention more after
each one.
Heh. So I originally wrote more on this in my earlier email, but worried
I was getting too into the weeds and would confuse you, so I pared it
down. But I managed to be confusing anyway. :)
Here's one mental model. If you have a temporary object store, then
these are the bad things that can happen:
1. Some code may write an object to it, thinking the data is saved,
but we later throw it away.
2. Some code may _skip_ writing an object, thinking that we already
have that object available (when in fact we'll later throw it
away).
3. Some code may record the fact that we have an object in memory, and
then later (after the temporary object store is not in use), may
point to it when writing an object or reference.
Pointing the primary object-dir at a temporary directory (like your
patch does) means we're subject to all three.
Problem (1) happens because unrelated code may not realize we've swapped
out the primary object writing code for this throw-away directory. But
we could require writers to explicitly indicate that they want to write
to the temporary area, while allowing other writes to go to the regular
object store. This could be done with a flag or variant of
write_object_file(). Or if the temporary area isn't a regular object
directory at all (like the whole tempfile / pretend thing), then this
happens automatically, because the regular object-write paths don't even
know about our temporary area.
Problem (2) is mostly handled inside write_object_file(). It will
optimize out writes of objects we already have. So it needs to know when
we have an object "for real", and when we just have it in our temporary
store. If we make the tmp_objdir the primary, then it doesn't know this
(though we could teach it to check. In the pretend_object_file() system,
it already knows this (it uses the "freshen" code paths which actually
want to find the entry in the filesystem).
Problem (3) is the hardest one, because we don't really distinguish
between readers and writers. So if I call has_object_file(), is it
because I want to access the object? Or is it because I want to generate
a new reference to it, which must ensure that it exists? One of those is
OK to look at the tmp_objdir (and indeed, is the whole point of making
it in the first place), and the other is the first step to corrupting
the repository. ;)
This is a good taxonomy. I think these problems apply to any transactional
system. If you keep state outside of the transaction, then you may wind up
with inconsistency. Problem (3) is the worst and kind of subsumes (2). If
we think an object is durable with a specific oid, we probably are going to
write that oid out somewhere else that is outside of the transaction. If
we write it to the ODB (e.g. in a tree or some blob) without first caching
it in memory, the references in the ODB will be equally durable as the referees.
With the environment variables we set up for running external commands
(like hooks) in the receive-pack quarantine, one bit of safety we have
is to prevent ref writes entirely in those commands. That works
reasonably well. Even if they may write new objects that might get
thrown away (if the push is rejected), they'd ultimately need to point
to those objects with a ref. And because those hooks spend their
_entire_ run-time in the quarantine state, they can't ever write a ref.
Whereas doing it in-process is a little dicier. Right now the textconv
cache writes out a new tree for every entry we add, so it happens to do
the ref write while the tmp-objdir is still in place. But as this
comment notes:
$ git grep -B6 notes_cache_write diff.c
diff.c- /*
diff.c- * we could save up changes and flush them all at the end,
diff.c- * but we would need an extra call after all diffing is done.
diff.c- * Since generating a cache entry is the slow path anyway,
diff.c- * this extra overhead probably isn't a big deal.
diff.c- */
diff.c: notes_cache_write(textconv->cache);
this is slow, and it would be perfectly reasonable to flush the cache at
the end of the process (e.g., using an atexit() handler). In which case
we'd hit this problem (3) exactly: we'd generate and remember objects
during the tmp-objdir period, but only later actually reference them.
This is more likely than the receive-pack hook case, because we're doing
it all in process.
Is there state we care about besides objects and refs? Maybe we need to
introduce a transactional system for the entire repository like we have
for the refs-db. In-process code that cares about oids that might be
only transactionally present can register a callback for when the transaction
commits or rollsback. I'm not volunteering to write this, btw :).
Right now, disabling Ref updates is a pretty good compromise, since that's the
one definite piece of durable state outside the ODB that can point in. In memory
state is generally hard to solve and could wind up persisting in either the ODB
or the Refs or both.
quoted
quoted
If you remove the tmp_objdir as the primary as soon as you're done with
the merge, but before you run the diff, you might be OK, though.
It has to be after I run the diff, because the diff needs access to
the temporary files to diff against them.
Right, of course. I was too fixated on the object-write part, forgetting
that the whole point of the exercise is to later read them back. :)
quoted
quoted
If not, then I think the solution is probably not to install this as the
"primary", but rather:
- do the specific remerge-diff writes we want using a special "write
to this object dir" variant of write_object_file()
- install the tmp_objdir as an alternate, so the reading side (which
is diff code that doesn't otherwise know about our remerge-diff
trickery) can find them
And that lets other writers avoid writing into the temporary area
accidentally.
Doesn't this have the same problem? If something similar to textconv
caching were to create new trees that referenced the existing blobs
and then added a ref that referred to that tree, then you have the
exact same problem, right? The new tree and the new ref would be
corrupt as soon as the tmp-objdir went away. Whether or not other
callers write to the temporary area isn't the issue, it's whether they
write refs that can refer to anything from the temporary area. Or am
I missing something?
The key thing here is in the first step, where remerge-diff is
explicitly saying "I want to write to this temporary object-dir". But
the textconv-cache code does not; it gets to write to the normal spot.
So it avoids problem (1).
You're right that it does not avoid problem (3) exactly. But triggering
that would require some code not just writing other objects or
references while the tmp-objdir is in place, but specifically
referencing the objects that remerge-diff did put into the tmp-objdir.
That seems a lot less likely to me (because the thing we're most worried
about is unrelated code that just happens to write while the tmp-objdir
is in place).
quoted
quoted
In that sense this is kind of like the pretend_object_file() interface,
except that it's storing the objects on disk instead of in memory. Of
course another way of doing that would be to stuff the object data into
tempfiles and just put pointers into the in-memory cached_objects array.
It's also not entirely foolproof (nor is the existing
pretend_object_file()). Any operation which is fooled into thinking we
have object X because it's in the fake-object list or the tmp_objdir may
reference that object erroneously, creating a corruption. But it's still
safer than allowing arbitrary writes into the tmp_objdir.
Why is the pretend_object_file() interface safer? Does it disable the
textconv caching somehow? I don't see why it helps avoid this
problem.
So hopefully it's clearer now from what I wrote above, but just
connecting the dots:
1. Unrelated code calling write_object_file() would still write real,
durable objects, as usual.
2. The write_object_file() "skip this write" optimization already
ignores the pretend_object_file() objects while checking "do we
have this object".
quoted
Interesting. I'm not familiar with the pretend_object_file() code,
but I think I catch what you're saying. I don't see anything in
object-file.c that allows removing items from the cache, as I would
want to do, but I'm sure I could add something.
Right, it's pretty limited now, and there's only one caller. And in fact
I've wanted to get rid of it, exactly because it can create the danger
we're discussing here. But I still think it's _less_ dangerous than
fully replacing the object directory.
quoted
(I'm still not sure how this avoids the problem of things like
textconv caching trying to write an object that references one of
these, though. Should we just manually disable textconv caching
during a remerge-diff?)
It can't avoid it completely, but in general, the textconv cache should
be writing and referencing its own objects. Even if they happen to be
the same as something remerge-diff generated, it won't know that until
it has tried to write it (and so that's why having write_object_file()
continue to really write the object is important).
Hopefully that clears up my line of thinking. I do think a lot of this
is kind of theoretical, in the sense that:
- textconv caching is an obscure feature that we _could_ just disable
during remerge-diffs
- there's a reasonable chance that there isn't any other code that
wants to write during a diff
But this is all scary and error-prone enough that I'd prefer an approach
that has the "least surprise". So any solution where random code calling
write_object_file() _can't_ accidentally write to a throw-away directory
seems like a safer less surprising thing.
There is something nice to be said for having the cached state go away
with the ephemeral objects, since that caching refers to objects that
may never be seen again.
Any solution which makes in-flight transactional state visible (either as
the primary writing spot or as a readable alternate) will suffer from state
leakage for non-transactionally-aware readers/writers.
The bulk-fsync code has some advantage in that it is more constrained in what
it's calling while the tmp objdir is active. Also, in typical cases, the
tmp objdir will be committed rather than discarded. But I believe it suffers
from the same essential issue.
Can we press forward, and just try to make sure all the in-process stuff
is aware of temporary object state? Maybe we can hook up the temp odb stuff
with the refsdb transactions into a single system.
We can make the tmp object directory stuff more prevalent by using it in
git-apply and the merge stategy, and by using bulk-checkin while writing out
the cached tree and commits. This should flush out any issues and keep other
parts of the codebase honest.
It does mean that the remerge-diff code needs to be explicit in its
object writes to say "this needs to go to the temporary area" (whether
it's a flag, or calling into a totally different function or subsystem).
I'm hoping that's not hard to do (because its writes are done explicitly
by the remerge-diff code), but I worry that it may be (because you are
relying on more generic tree code to the write under the hood).
I don't think that really helps the trickiest problems. The textconv cache,
for instance, may now be writing notes that refer to nonexistent objects.
That's fine from a correctness point of view, but now we're polluting the repo
with state that won't be gc'ed and won't ever be accessed.
IMO, it might be better for the textconv cache to have a single notes root
tree (rather than a notes 'branch' with a commit history). We would want to
put a ref to this tree somewhere inside the object directory itself so that
it's transactionally consistent with the rest of the object state. For instance,
we could store the current textconv notes tree root in .git/objects/info/textconv-cache.
Thanks,
Neeraj
On Thu, Sep 30, 2021 at 9:53 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
quoted
@@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' ' printf "\0\0" >binary-file && git add binary-file && git commit -m binary2 &&- test_must_fail git merge F >merge.out 2>merge.err &&- grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort+ then+ test_must_fail git merge F >merge.out &&+ grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out+ else+ test_must_fail git merge F >merge.out 2>merge.err &&+ grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err+ fi '
To save readers from eyeballing if a single character has changed here,
which I don't think it has, just do:
if ...
then
cmd >actual
else
other cmd >actual
Do you mean
other cmd >/dev/null 2>actual
?
fi &&
grep [...]
I.e. no need to duplicate the "grep" here just because of merge.out v.s. merge.err.
[...]
quoted
- test_must_fail git merge bin-main 2>stderr &&
- grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort
+ then
+ test_must_fail git merge bin-main >stdout &&
+ grep -i "warning.*cannot merge.*HEAD vs. bin-main" stdout
+ else
+ test_must_fail git merge bin-main 2>stderr &&
+ grep -i "warning.*cannot merge.*HEAD vs. bin-main" stderr
+ fi
On Thu, Sep 30, 2021 at 12:58 AM Jeff King [off-list ref] wrote:
On Tue, Sep 28, 2021 at 11:25:20PM -0700, Elijah Newren wrote:
quoted
quoted
Just brainstorming some alternatives:
- we have diff.orderFile, etc. Could we stuff this data into a less
confusing name (even just "$filename.conflict_msg"), and then provide
a custom ordering to the diff code? I think it could be done by
generating a static ordering ahead of time, but it might even just be
possible to tell diffcore_order() to take the ".conflict_msg"
extension into account in its comparison function.
I can't just go on the ".conflict_msg" extension. As you noted above,
this scheme is not sufficient for avoiding collisions. So I need to
append extra "cruft" to the name in the case of collisions -- meaning
we can't special case on just that extension.
Sure, but we can call it filename.conflict_msg.1, etc, and the sort code
can pattern-match. It can never be fully robust (if you really did have
a foo.conflict_msg, we'd sort it differently), but I think it's OK if
the worst-case is that pathological trees get ordered slightly
sub-optimally).
That said, I think it could also make sense to annotate the conflict
files by giving them an unusual set of mode bits. That would be easier
to recognize (and while real trees _could_ have silly modes, we do
complain about them in fsck, so it shouldn't happen in practice).
I tried giving things unusual mode bits in early versions of merge-ort
for other reasons. It doesn't work: canon_mode() will "fix" the
unusualness so there's no way for the reader to know that they had
unusual bits.
But, as you said later, avoiding these pseudo-files is going to be
cleaner anyway, so let's just do that.
quoted
I also don't like how diff.orderFile provides a global ordering of the
files listed, rather than providing some scheme for relative
orderings. That'd either force me to precompute the diff to determine
all the files that were different so I can list _all_ of them, or put
up with the fact that the files with non-content conflicts will be
listed very first in the output, even if their name is
'zee-last-file.c' -- surprising users at the output ordering.
This also means that if the user had their own ordering defined, then
I'm overriding it and messing up their ordering, which might be
problematic.
Agreed. I do think it may be too horrible unless you teach
diffcore_order() to actually understand your annotations in code. But
that wouldn't be too hard if it's done in the mode bits.
But I do think anything that avoids these pseudo-files is going to be a
lot cleaner overall.
quoted
quoted
- there can be other non-diff data between the individual segments. For
example, "patch" will skip over non-diff lines. And certainly in Git
we have our own custom headers. I'm wondering if we could attach
these annotations to the diff-pair somehow, and then show something
like:
diff --git a/foo.c b/foo.c
index 1234abcd..5678cdef 100644
conflict modify/delete foo.c
A couple things here...
I think Junio already indicated that we can pretty much make this look
like whatever we want. As soon as any reader sees "conflict", it should
happily ignore the rest as something it doesn't know about. And my short
example here was just meant to be illustrative. I agree it probably
needs more details (and the whole CONFLICT line that usually goes to
stderr is probably the best thing).
-Peff
On Thu, Sep 30, 2021 at 12:26 AM Jeff King [off-list ref] wrote:
On Tue, Sep 28, 2021 at 10:05:31PM -0700, Elijah Newren wrote:
quoted
So there are other parts of running `git log` that are not read-only,
besides what I'm doing. That's unfortunate. Can we just disable
those things in this section of the code?
It might be possible for the cache (we'd still generate entries, but
just not write them). It's "just" an optimization, though it's possible
there are cases where it's really annoying to do so. I'd imagine those
are a stretch. I do use textconv to decrypt gpg blobs with a key on a
hardware token, for example. I don't require a touch for each one,
though, nor would I use textconv caching anyway (since the point is not
to store the unencrypted contents).
It wouldn't necessarily be OK to disable object writes in general.
Though I'd think since a diff is _conceptually_ a read-only operation
that any writes it wants to do would be in the same caching /
optimization boat. I'd also worry about the maintenance cost of having
to annotate any such case.
quoted
Also, I think it makes sense that textconv caching causes a problem
via writing new blobs and trees *and* refs that reference these blobs
and trees. However, I'm not sure I'm really grasping it, because I
don't see how your solutions are safe either. I'll mention more after
each one.
Heh. So I originally wrote more on this in my earlier email, but worried
I was getting too into the weeds and would confuse you, so I pared it
down. But I managed to be confusing anyway. :)
Here's one mental model. If you have a temporary object store, then
these are the bad things that can happen:
1. Some code may write an object to it, thinking the data is saved,
but we later throw it away.
2. Some code may _skip_ writing an object, thinking that we already
have that object available (when in fact we'll later throw it
away).
3. Some code may record the fact that we have an object in memory, and
then later (after the temporary object store is not in use), may
point to it when writing an object or reference.
Pointing the primary object-dir at a temporary directory (like your
patch does) means we're subject to all three.
Problem (1) happens because unrelated code may not realize we've swapped
out the primary object writing code for this throw-away directory. But
we could require writers to explicitly indicate that they want to write
to the temporary area, while allowing other writes to go to the regular
object store. This could be done with a flag or variant of
write_object_file(). Or if the temporary area isn't a regular object
directory at all (like the whole tempfile / pretend thing), then this
happens automatically, because the regular object-write paths don't even
know about our temporary area.
Problem (2) is mostly handled inside write_object_file(). It will
optimize out writes of objects we already have. So it needs to know when
we have an object "for real", and when we just have it in our temporary
store. If we make the tmp_objdir the primary, then it doesn't know this
(though we could teach it to check. In the pretend_object_file() system,
it already knows this (it uses the "freshen" code paths which actually
want to find the entry in the filesystem).
Problem (3) is the hardest one, because we don't really distinguish
between readers and writers. So if I call has_object_file(), is it
because I want to access the object? Or is it because I want to generate
a new reference to it, which must ensure that it exists? One of those is
OK to look at the tmp_objdir (and indeed, is the whole point of making
it in the first place), and the other is the first step to corrupting
the repository. ;)
With the environment variables we set up for running external commands
(like hooks) in the receive-pack quarantine, one bit of safety we have
is to prevent ref writes entirely in those commands. That works
reasonably well. Even if they may write new objects that might get
thrown away (if the push is rejected), they'd ultimately need to point
to those objects with a ref. And because those hooks spend their
_entire_ run-time in the quarantine state, they can't ever write a ref.
Whereas doing it in-process is a little dicier. Right now the textconv
cache writes out a new tree for every entry we add, so it happens to do
the ref write while the tmp-objdir is still in place. But as this
comment notes:
$ git grep -B6 notes_cache_write diff.c
diff.c- /*
diff.c- * we could save up changes and flush them all at the end,
diff.c- * but we would need an extra call after all diffing is done.
diff.c- * Since generating a cache entry is the slow path anyway,
diff.c- * this extra overhead probably isn't a big deal.
diff.c- */
diff.c: notes_cache_write(textconv->cache);
this is slow, and it would be perfectly reasonable to flush the cache at
the end of the process (e.g., using an atexit() handler). In which case
we'd hit this problem (3) exactly: we'd generate and remember objects
during the tmp-objdir period, but only later actually reference them.
This is more likely than the receive-pack hook case, because we're doing
it all in process.
quoted
quoted
If you remove the tmp_objdir as the primary as soon as you're done with
the merge, but before you run the diff, you might be OK, though.
It has to be after I run the diff, because the diff needs access to
the temporary files to diff against them.
Right, of course. I was too fixated on the object-write part, forgetting
that the whole point of the exercise is to later read them back. :)
quoted
quoted
If not, then I think the solution is probably not to install this as the
"primary", but rather:
- do the specific remerge-diff writes we want using a special "write
to this object dir" variant of write_object_file()
- install the tmp_objdir as an alternate, so the reading side (which
is diff code that doesn't otherwise know about our remerge-diff
trickery) can find them
And that lets other writers avoid writing into the temporary area
accidentally.
Doesn't this have the same problem? If something similar to textconv
caching were to create new trees that referenced the existing blobs
and then added a ref that referred to that tree, then you have the
exact same problem, right? The new tree and the new ref would be
corrupt as soon as the tmp-objdir went away. Whether or not other
callers write to the temporary area isn't the issue, it's whether they
write refs that can refer to anything from the temporary area. Or am
I missing something?
The key thing here is in the first step, where remerge-diff is
explicitly saying "I want to write to this temporary object-dir". But
the textconv-cache code does not; it gets to write to the normal spot.
So it avoids problem (1).
You're right that it does not avoid problem (3) exactly. But triggering
that would require some code not just writing other objects or
references while the tmp-objdir is in place, but specifically
referencing the objects that remerge-diff did put into the tmp-objdir.
That seems a lot less likely to me (because the thing we're most worried
about is unrelated code that just happens to write while the tmp-objdir
is in place).
quoted
quoted
In that sense this is kind of like the pretend_object_file() interface,
except that it's storing the objects on disk instead of in memory. Of
course another way of doing that would be to stuff the object data into
tempfiles and just put pointers into the in-memory cached_objects array.
It's also not entirely foolproof (nor is the existing
pretend_object_file()). Any operation which is fooled into thinking we
have object X because it's in the fake-object list or the tmp_objdir may
reference that object erroneously, creating a corruption. But it's still
safer than allowing arbitrary writes into the tmp_objdir.
Why is the pretend_object_file() interface safer? Does it disable the
textconv caching somehow? I don't see why it helps avoid this
problem.
So hopefully it's clearer now from what I wrote above, but just
connecting the dots:
1. Unrelated code calling write_object_file() would still write real,
durable objects, as usual.
2. The write_object_file() "skip this write" optimization already
ignores the pretend_object_file() objects while checking "do we
have this object".
quoted
Interesting. I'm not familiar with the pretend_object_file() code,
but I think I catch what you're saying. I don't see anything in
object-file.c that allows removing items from the cache, as I would
want to do, but I'm sure I could add something.
Right, it's pretty limited now, and there's only one caller. And in fact
I've wanted to get rid of it, exactly because it can create the danger
we're discussing here. But I still think it's _less_ dangerous than
fully replacing the object directory.
quoted
(I'm still not sure how this avoids the problem of things like
textconv caching trying to write an object that references one of
these, though. Should we just manually disable textconv caching
during a remerge-diff?)
It can't avoid it completely, but in general, the textconv cache should
be writing and referencing its own objects. Even if they happen to be
the same as something remerge-diff generated, it won't know that until
it has tried to write it (and so that's why having write_object_file()
continue to really write the object is important).
Hopefully that clears up my line of thinking. I do think a lot of this
is kind of theoretical, in the sense that:
- textconv caching is an obscure feature that we _could_ just disable
during remerge-diffs
- there's a reasonable chance that there isn't any other code that
wants to write during a diff
But this is all scary and error-prone enough that I'd prefer an approach
that has the "least surprise". So any solution where random code calling
write_object_file() _can't_ accidentally write to a throw-away directory
seems like a safer less surprising thing.
Yes, thanks for taking the time to go into this detail.
It does mean that the remerge-diff code needs to be explicit in its
object writes to say "this needs to go to the temporary area" (whether
it's a flag, or calling into a totally different function or subsystem).
I'm hoping that's not hard to do (because its writes are done explicitly
by the remerge-diff code), but I worry that it may be (because you are
relying on more generic tree code to the write under the hood).
All blob and tree objects written during merge-ort are done by
explicit write_object_file() calls that exist within merge-ort.c. So
this approach should be doable, it just needs the external caller to
set some flag saying to write these objects elsewhere.
On Thu, Sep 30, 2021 at 12:46 AM Jeff King [off-list ref] wrote:
On Thu, Sep 30, 2021 at 03:26:42AM -0400, Jeff King wrote:
quoted
quoted
quoted
If you remove the tmp_objdir as the primary as soon as you're done with
the merge, but before you run the diff, you might be OK, though.
It has to be after I run the diff, because the diff needs access to
the temporary files to diff against them.
Right, of course. I was too fixated on the object-write part, forgetting
that the whole point of the exercise is to later read them back. :)
Ah, no, I remember what I was trying to say here. The distinction is
between "remove the tmp_objdir" and "remove it as the primary".
I.e., if you do this:
1. create tmp_objdir
2. make tmp_objdir primary for writes
3. run the "merge" half of remerge-diff, writing objects into the
temporary space
4. stop having tmp_objdir as the primary; instead make it an alternate
5. run the diff
6. remove tmp_objdir totally
Then step 5 can't accidentally write objects into the temporary space,
but it can still read them. So it's not entirely safe, but it's safer,
and it would be a much smaller change.
Interesting.
Some ways it could go wrong:
- is it possible for the merge code to ever write an object? I kind of
wonder if we'd ever do any cache-able transformations as part of a
content-level merge. I don't think we do now, though.
Yes, of course -- otherwise there would have been no need for the
tmp_objdir in the first place. In particular, it needs to write
three-way-content merges of individual files, and it needs to write
new tree objects. (And it needs to do this both for creating the
virtual merge bases if the merge is recursive, as well as doing it for
the outer merge.)
It doesn't write anything for caching reasons, such as line ending
normalizations (that's all kept in-memory and done on demand).
- in step 5, write_object_file() may still be confused by the presence
of the to-be-thrown-away objects in the alternate. This is pretty
unlikely, as it implies that the remerge-diff wrote a blob or tree
that is byte-identical to something that the diff wants to write.
That's one reason it could be confused. The textconv filtering in
particular was creating a new object based on an existing one, and a
tree, and a ref. What if there was some other form of caching or
statistic gathering that didn't write a new object based on an
existing one, but did add trees and especially refs that referenced
the existing object? It's not that the diff wanted to write something
byte-identical to what the merge wrote, it's just that the diff wants
to reference the object somehow.
Not your fault & this pre-dates your patch, but FWIW I prefer our APIs
that don't have these "hidden struct" shenanigans (like say "struct
string_list") so a caller could just access this, we can just declare it
"const" appropriately.
We're also all in-tree friends here, so having various accessors for no
reason other than to access struct members seems a bit too much.
That's a fair point, but just to play counterpoint for a minute...
FWIW, I dislike when our public facing APIs are polluted with all
kinds of internal details. merge-recursive being a case in point.
When writing merge-ort, although I had a struct with public fields,
that struct also contained an opaque struct (pointer) within it to
hide several private fields. (I would have liked to hide or remove a
few more fields, but couldn't do so while the merge_recursive_options
struct was shared between both merge-ort and merge-recursive.)
That said, I agree it can certainly be overdone, and tmp_objdir is
pretty simple. However, sometimes even in simple cases I like when
folks make use of an opaque struct because it means folks put some
effort into thinking more about the API that should be exposed.
That's something we as a project have often overlooked in the past, as
evidenced both by our one-shot usage mentality, and the existence of
external projects like libgit2 attempting to correct this design
shortcoming. I'd like git to move more towards being structured as a
reusable library as well as a useful command-line tool.
[This is somewhat of a continuation of
https://lore.kernel.org/git/87lf3etaih.fsf@evledraar.gmail.com/].
I like our APIs being usable, but right now we're not promising any
maintenance of APIs that work the same as git v2.30.0 or whatever.
I think some external users directly link to libgit.a, but they're
tracking whatever churn we have from release to release, just like
someone maintaining out-of-tree linux kernel drivers.
For maintenance of git.git itself I think it's going to stay like that
for any foreseeable future[1].
And that's before we even get into the practicalities of git's license
likely not winning over many (if any) libgit2 users. I think those users
are if anything migrating from libgit2 to git's plumbing CLI, or in some
cases folding what they needed into git.git itself (or just staying with
libgit2).
So assuming all of that I really don't see the point in general of
having a "foo" field that has the equivalent of get_foo() and set_foo()
accessors in the context of git.git.
That's something that's really useful if you're maintaining API and ABI
compatibility, but for us it's usually just verbosity. All the users are
in-tree, so just add "const" as appropriate, or perhaps we should give
the field a "priv" name or whatever.
It wasn't just a get or set accessors, and there was more involved
than just adding const:
* There was a typechange from struct strbuf -> const char *; strbuf != char *
* Having an API that _enforces_ accesses to be const provides more
safety. Relying on callers to remember to declare their variable
access as const provides virtually none.
But more importantly than either of the above, tmp_objdir was locked
down because it was too easy to make big mistakes with, as Peff
pointed out. That meant that folks would only use it through the API,
and if folks tried to do unusual things, they'd do so by adding new
API, which is easier to flag in code reviews and make sure the right
people get cc'ed than if folks just reach into some internals from
some other file. And the new API that I added resulted in a
discussion that pointed out I didn't need to access that field at all
and should instead create and delete the tmp_objdir after every merge.
Obviously not everything should be done that way, e.g. being able to
have trace2 start/end points for something you'd otherwise push/clear
from a "struct string_list" or whatever directly *is* usable, so it's
not black and white.
I'm just saying thaht using convoluted patterns *just* to get in the way
of some hypothetical code that's going to mess with your internals can
make things harder in other areas. E.g. the init/release pattern via
macros I mentioned in the linked E-Mail above.
Fair points, but I just don't think that's relevant to the context
here; i.e. these aren't convoluted patterns *just* to get in the way.
Further, I don't see how this would adversely affect the *_INIT macros
you mentioned at all. tmp_objdir is an opaque struct so you can only
declare a pointer to it. Pointers being initialized to NULL is
handled quite nicely by the *_INIT macros.
quoted
[...]
quoted
In this particular case I hadn't understood on a previous reading of
tmp-objdir.c why this "path" isn't a statically allocated "char
path[PATH_MAX]", or a least we that hardcoded "1024" should be a
PATH_MAX, as it is in some other cases.
Thanks. I didn't know about that. I've fixed (some of that) up in some
semi-related WIP work I've got.
1. Aside: I *do* think it would be really useful for us to expose a
stable C API eventually, using some of the same license
workarounds/shenanigans (depending on your view) as the Linux kernel
did. I.e. to say that "we're GPLv2, but this small API is the
equivalent of our syscall interface".
Rather than that API being something that would look anything like
internal git.git users it would basically be a run_command() + Perl's
unpack/pack + output formatter on steroids.
I.e. we'd be able to invoke say a 'git cat-file --batch' in the same
process as the caller by essentially calling cmd_cat_file()
directly. Just with some callback replacements for "provide this on
stdin" and "this line accepts stdout" without actually involving
different processes or I/O.
You'd then have the equivalent of a --format output that would just
so happen to be in your OS's / C ABI's understood struct
format. Similar to "perldoc -f pack".
The maintenance burden of such an API would be trivial. It would
basically be a handful of functions just to setup input/output
callbacks.
Most of the interface would be a rather convoluted construction of
command-line parameters (like our own run_command() use), not ideal,
but most users are after structured data and eliminating process
overhead, and they'd get that.
We would need to replace fprintf() and the like with some structured
formatting mechanism, but that would be useful for non-API users as
new commands learning a shiny new --format parameter. You could even
use such a --format to have a non-C-API user get the same struct
output, perhaps for direct mapping into memory, say if you didn't
trust the built-in/command in question to not leak memory yet.
I don't know if this has been suggested before, just my 0.02. It's
also partially why I'm interested in making even-built-ins not leak
memory with some of my parallel SANITIZE=leak work.
Once you've got that you can usually call the cmd_whatever() in a
loop without much effort, and once you've got that, well, see above
:)
From: Jeff King <hidden> Date: 2021-10-01 05:21:49
On Thu, Sep 30, 2021 at 07:31:44PM -0700, Elijah Newren wrote:
quoted
Some ways it could go wrong:
- is it possible for the merge code to ever write an object? I kind of
wonder if we'd ever do any cache-able transformations as part of a
content-level merge. I don't think we do now, though.
Yes, of course -- otherwise there would have been no need for the
tmp_objdir in the first place. In particular, it needs to write
three-way-content merges of individual files, and it needs to write
new tree objects. (And it needs to do this both for creating the
virtual merge bases if the merge is recursive, as well as doing it for
the outer merge.)
Right, sorry, I should have added: ...in addition to the merge-results
we're writing. What I'm getting at is whether there might be _other_
parts of the code that the merge code would invoke. Say, if a
.gitattribute caused us to convert a file's encoding in order to perform
a more semantic merge, and we wanted to store that result somehow (e.g.,
in a similar cache).
I don't think anything like that exists now, but I don't find it outside
the realm of possibility in the future. It's sort of the merge
equivalent of "textconv"; we have external diff and external merge
drivers, but being able to convert contents and feed them to the regular
text diff/merge code simplifies things.
quoted
- in step 5, write_object_file() may still be confused by the presence
of the to-be-thrown-away objects in the alternate. This is pretty
unlikely, as it implies that the remerge-diff wrote a blob or tree
that is byte-identical to something that the diff wants to write.
That's one reason it could be confused. The textconv filtering in
particular was creating a new object based on an existing one, and a
tree, and a ref. What if there was some other form of caching or
statistic gathering that didn't write a new object based on an
existing one, but did add trees and especially refs that referenced
the existing object? It's not that the diff wanted to write something
byte-identical to what the merge wrote, it's just that the diff wants
to reference the object somehow.
Yes, good point. It can help a little if the alternate-odb added by
tmp_objdir was marked with a flag to say "hey, this is temporary". That
would help write_object_file() decide not to depend on it. But the
problem is so much wider than that that I think it will always be a bit
dangerous; any code could say "can I access this object" and we don't
know if its intent is simply to read it, or to create a new object that
references it.
-Peff
From: Jeff King <hidden> Date: 2021-10-01 05:28:49
On Thu, Sep 30, 2021 at 07:07:21PM -0700, Elijah Newren wrote:
quoted
That said, I think it could also make sense to annotate the conflict
files by giving them an unusual set of mode bits. That would be easier
to recognize (and while real trees _could_ have silly modes, we do
complain about them in fsck, so it shouldn't happen in practice).
I tried giving things unusual mode bits in early versions of merge-ort
for other reasons. It doesn't work: canon_mode() will "fix" the
unusualness so there's no way for the reader to know that they had
unusual bits.
Ah, right, I totally forgot about that.
But, as you said later, avoiding these pseudo-files is going to be
cleaner anyway, so let's just do that.
I'm quite happy if you're on board with that direction. :)
-Peff
On Thu, Sep 30, 2021 at 9:53 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
quoted
On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
quoted
@@ -108,8 +108,14 @@ test_expect_success 'refuse to merge binary files' ' printf "\0\0" >binary-file && git add binary-file && git commit -m binary2 &&- test_must_fail git merge F >merge.out 2>merge.err &&- grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err+ if test "$GIT_TEST_MERGE_ALGORITHM" = ort+ then+ test_must_fail git merge F >merge.out &&+ grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.out+ else+ test_must_fail git merge F >merge.out 2>merge.err &&+ grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err+ fi '
To save readers from eyeballing if a single character has changed here,
which I don't think it has, just do:
if ...
then
cmd >actual
else
other cmd >actual
Do you mean
other cmd >/dev/null 2>actual
?
Yes, sorry. I was going for lazy pseudocode. I meant maybe the "grep"
can be shared between the two.