git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Fabian Stelzer <fs@gigacodes.de>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Christian Couder <chriscool@tuxfamily.org>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH 7/8] merge-tree: support saving merge messages to a separate file
Date: Mon, 3 Jan 2022 11:46:53 -0800	[thread overview]
Message-ID: <CABPp-BEneaLnaTVo-Yb7fooLK8sQsDq_MdeBu-R2EporqCSqoQ@mail.gmail.com> (raw)
In-Reply-To: <20220103172255.54psh2e5iqzd37sy@fs>

On Mon, Jan 3, 2022 at 9:23 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>
> On 03.01.2022 08:51, Elijah Newren wrote:
> >On Mon, Jan 3, 2022 at 4:31 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> >>
> >> On 31.12.2021 05:04, Elijah Newren via GitGitGadget wrote:
> >> >From: Elijah Newren <newren@gmail.com>
> [...]
> >> >
> >> > static int real_merge(struct merge_tree_options *o,
> >> >@@ -442,8 +443,15 @@ static int real_merge(struct merge_tree_options *o,
> >> >        */
> >> >
> >> >       merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
> >> >+
> >> >+      if (o->messages_file) {
> >> >+              FILE *fp = xfopen(o->messages_file, "w");
> >> >+              merge_display_update_messages(&opt, &result, fp);
> >> >+              fclose(fp);
> >>
> >> I don't know enough about how merge-ort works internally, but it looks to me
> >> like at this point the merge already happened and we just didn't clean up
> >> (finalize) yet. It feels wrong to die() at this point just because we can't
> >> open messages_file.
> >
> >Yes, the merge already happened; there now exists a new toplevel tree
> >(that nothing references).  I'm not sure I understand what's wrong
> >with die'ing here, though.  I can't tell if you want to defer the
> >die-ing until later, or just avoid the die-ing and return some kind of
> >success despite failing to complete what the user requested.
> >
>
> I think i would prefer the merge operation to abort before actually merging
> when not being able to write its logfile. Otherwise we possibly do a whole
> lot of work that`s inaccessible afterwards isn't it? (since we don`t print
> the hash)

I see where you're coming from, but I don't see this as worth worrying
about.  For two reasons:

(1) I'm not sure I buy the "whole lot of work" concern.

merge-ort is pretty snappy.  For a simple example of rebasing a single
patch in linux.git across a branch with 28000 renames, I get 176
milliseconds for merge_incore_nonrecursive().  Granted, linux.git is
pretty small in terms of number of files, but Stolee did some
measurements a while back on the Microsoft repos with millions of
files at HEAD.  For those, for a trivial merge he saw
merge_incore_recursive() complete in 2 milliseconds, and for a trivial
rebase he saw merge_incore_nonrecursive() complete in 4 milliseconds
(See https://lore.kernel.org/git/CABPp-BHO7bZ3H7A=E9TudhvBoNfwPvRiDMm8S9kq3mYeSXrpXw@mail.gmail.com/).
So huge numbers of files pose much less of a problem than lots of
interesting work like renames, and merge-ort is pretty fast in either
case.  Sure, if we were talking about traditional merge-recursive
which would have taken 150000 milliseconds on the same single patch in
linux.git testcase (due to the 28000 renames), then we might worry
more about not letting work get tossed, but at only 176 milliseconds
even with a crazy number of renames, it's just not worth worrying
about.

(2) Even if there is a lot of computation, I don't see why this error
path merits extra coding work to salvage the computation somehow

By way of comparison, a regular `git merge` will abort after
completing the same amount of merge work (i.e. after creating a new
tree) when the user has a dirty working tree involving a path that
would need to be updated by the merge operation.  And that is not a
bug; it's a requirement -- we cannot first check if the user has
dirtied such a path before performing the merge because it's
impossible to do so accurately in the face of renames.
merge-recursive tried to do that and had early aborts that fell in the
false-positive category and some that fell in the false-negative
category.  It was impossible to fix the false-positives and
false-negatives without either (a) disallowing ever doing a merge with
a dirty working tree under any conditions (a backwards compatibility
break), or (b) waiting to do the notification of
dirty-files-in-the-way until after the merge tree has been computed.
I wasn't about to break that feature, so merge-ort had to delay error
notifications instead.

Now, the dirty-file-in-the-way condition is for a very common case
(either for users who intentionally like keeping dirty changes around
and doing merges but the branch they are merging happens to touch a
file they didn't know about, or users who just forgot that they had
local modifications).  In contrast, this case here is for when we
cannot open a file for writing -- with the filename explicitly just
specified by the user.


So, I'd rather keep the code nice and simple as it currently stands.

> Thanks for your work on this feature. I think this could open a lot of new
> possibilities.

I hope people do interesting things with it, and with the server-side
commit replaying I'm working on as well.

  reply	other threads:[~2022-01-03 19:47 UTC|newest]

Thread overview: 57+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-31  5:03 [PATCH 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
2021-12-31  5:03 ` [PATCH 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
2021-12-31  5:03 ` [PATCH 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
2022-01-01 20:11   ` Johannes Altmanninger
2022-01-01 20:17     ` Elijah Newren
2021-12-31  5:03 ` [PATCH 3/8] merge-tree: add option parsing and initial shell for real merge function Elijah Newren via GitGitGadget
2021-12-31  5:04 ` [PATCH 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
2022-01-01 20:08   ` Johannes Altmanninger
2022-01-01 21:11     ` Elijah Newren
2022-01-03 12:23   ` Fabian Stelzer
2022-01-03 16:37     ` Elijah Newren
2021-12-31  5:04 ` [PATCH 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
2022-01-03 12:15   ` Fabian Stelzer
2022-01-03 12:25     ` Fabian Stelzer
2021-12-31  5:04 ` [PATCH 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
2022-01-01 20:08   ` Johannes Altmanninger
2022-01-01 20:19     ` Elijah Newren
2021-12-31  5:04 ` [PATCH 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
2022-01-03 12:31   ` Fabian Stelzer
2022-01-03 16:51     ` Elijah Newren
2022-01-03 17:22       ` Fabian Stelzer
2022-01-03 19:46         ` Elijah Newren [this message]
2022-01-04 13:05           ` Fabian Stelzer
2022-01-03 12:35   ` Fabian Stelzer
2022-01-03 16:55     ` Elijah Newren
2021-12-31  5:04 ` [PATCH 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
2022-01-05 17:27 ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 1/8] merge-tree: rename merge_trees() to trivial_merge_trees() Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 2/8] merge-tree: move logic for existing merge into new function Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 3/8] merge-tree: add option parsing and initial shell for real merge function Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 4/8] merge-tree: implement real merges Elijah Newren via GitGitGadget
2022-01-07 15:30     ` Johannes Schindelin
2022-01-07 17:26       ` Elijah Newren
2022-01-07 18:22         ` Johannes Schindelin
2022-01-07 19:15           ` Elijah Newren
2022-01-07 20:56         ` Junio C Hamano
2022-01-11 13:39           ` Johannes Schindelin
2022-01-07 18:12     ` Christian Couder
2022-01-07 19:09       ` Elijah Newren
2022-01-05 17:27   ` [PATCH v2 5/8] merge-ort: split out a separate display_update_messages() function Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 6/8] merge-ort: allow update messages to be written to different file stream Elijah Newren via GitGitGadget
2022-01-05 17:27   ` [PATCH v2 7/8] merge-tree: support saving merge messages to a separate file Elijah Newren via GitGitGadget
2022-01-07 18:07     ` Johannes Schindelin
2022-01-08  1:02       ` Elijah Newren
2022-01-05 17:27   ` [PATCH v2 8/8] merge-tree: provide an easy way to access which files have conflicts Elijah Newren via GitGitGadget
2022-01-05 19:09     ` Ramsay Jones
2022-01-05 19:17       ` Elijah Newren
2022-01-07 19:36     ` Johannes Schindelin
2022-01-07 22:12       ` Elijah Newren
2022-02-22 13:03         ` Johannes Schindelin
2022-01-08  1:28       ` Elijah Newren
2022-02-22 13:05         ` Johannes Schindelin
2022-01-05 20:18   ` [PATCH v2 0/8] RFC: Server side merges (no ref updating, no commit creating, no touching worktree or index) Junio C Hamano
2022-01-05 22:35     ` Elijah Newren
2022-01-07 18:46   ` Christian Couder
2022-01-07 19:59     ` Elijah Newren
2022-01-07 21:26       ` René Scharfe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABPp-BEneaLnaTVo-Yb7fooLK8sQsDq_MdeBu-R2EporqCSqoQ@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=fs@gigacodes.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=me@ttaylorr.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).