merge-tree: add new --quiet option #1920
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Elijah Newren via GitGitGadget" <[email protected]> writes:
> This adds a new flag, --mergeability-only, to git merge-tree, which
> suppresses all output and leaves only the exit status (reflecting successful
> merge or conflict). This is useful for Git Forges in cases where they are
> only interested in whether two branches can be merged, without needing the
> actual merge result or conflict details.
Sounds useful, but wouldn't that usually called --dry-run?
|
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Mon, May 12, 2025 at 10:04 AM Junio C Hamano <[email protected]> wrote:
>
> "Elijah Newren via GitGitGadget" <[email protected]> writes:
>
> > This adds a new flag, --mergeability-only, to git merge-tree, which
> > suppresses all output and leaves only the exit status (reflecting successful
> > merge or conflict). This is useful for Git Forges in cases where they are
> > only interested in whether two branches can be merged, without needing the
> > actual merge result or conflict details.
>
> Sounds useful, but wouldn't that usually called --dry-run?
I thought about that, but I was worried that folks would expect
"--dry-run" to not make any changes. This mode does not prevent
writing objects to the object store, it merely avoids it in the "outer
layer" of the merge. More precisely, objects will still be written to
the object store for the merging of merge bases, and also be written
to the object store in the case of rename/rename conflicts if the
contents of the files involved in the conflicting renames were also
modified by both sides. |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Elijah Newren <[email protected]> writes:
> I thought about that, but I was worried that folks would expect
> "--dry-run" to not make any changes. This mode does not prevent
> writing objects to the object store, it merely avoids it in the "outer
> layer" of the merge.
I think we have already precedence to call something that creates
new objects in the object database, as long as the resulting objects
are not made reachable ("git fetch --dry-run" probably falls into
that category). The idea is that it does not make a change that is
"observable" by end-users (and what "gc" sees is not part of what
the users would be observaing).
We have "--check" (in "git apply"), which is an exact counterpart in
the patch based workflow to this thing. It reads
Instead of applying the patch, see if the patch is
applicable to the current working tree and/or the index
file and detects errors. Turns off "apply".
I feel that `apply --check` should have been `apply --dry-run`, so I
would not recommend calling it `--check` for `merge-tree`, though.
Thanks. |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Mon, May 12, 2025 at 11:27 AM Junio C Hamano <[email protected]> wrote:
>
> Elijah Newren <[email protected]> writes:
>
> > I thought about that, but I was worried that folks would expect
> > "--dry-run" to not make any changes. This mode does not prevent
> > writing objects to the object store, it merely avoids it in the "outer
> > layer" of the merge.
>
> I think we have already precedence to call something that creates
> new objects in the object database, as long as the resulting objects
> are not made reachable ("git fetch --dry-run" probably falls into
> that category). The idea is that it does not make a change that is
> "observable" by end-users (and what "gc" sees is not part of what
> the users would be observaing).
Oh, I was unaware of `git fetch --dry-run` for some reason. And its
documentation even states "without making any changes" despite the
fact that it downloads more objects to the object store, so it indeed
sounds like a good precedent.
I'll switch the flag name to --dry-run. (I have a suspicion, however,
that the primary users of this new merge-tree flag will care about
whether objects are created, so I still want the documentation to call
it out, unlike git fetch's --dry-run option.)
> We have "--check" (in "git apply"), which is an exact counterpart in
> the patch based workflow to this thing. It reads
>
> Instead of applying the patch, see if the patch is
> applicable to the current working tree and/or the index
> file and detects errors. Turns off "apply".
>
> I feel that `apply --check` should have been `apply --dry-run`, so I
> would not recommend calling it `--check` for `merge-tree`, though.
Makes sense; thanks for the pointers. |
Git Forges may be interested in whether two branches can be merged while
not being interested in what the resulting merge tree is nor which files
conflicted. For such cases, add a new mergeability_only option. This
option allows the merge machinery to, in the "outer layer" of the merge:
* exit upon first[-ish] conflict
* avoid (not prevent) writing merged blobs/trees to the object store
I have a number of qualifiers there, so let me explain each:
"outer layer":
Note that since the recursive merge of merge bases (corresponding to
call_depth > 0) can conflict without the outer final merge
(corresponding to call_depth == 0) conflicting, we can't short-circuit
nor avoid writing merged blobs/trees to the object store during those
inner merges.
"first-ish conflict":
The current patch only exits early from process_entries() on the first
conflict it detects, but conflicts could have been detected in a
previous function call, namely detect_and_process_renames(). However:
* conflicts detected by detect_and_process_renames() are quite rare
conflict types
* the detection would still come after regular rename detection
(which is the expensive part of detect_and_process_renames()), so
it is not saving us much in computation time given that
process_entries() directly follows detect_and_process_renames()
* [this overlaps with the next bullet point] process_entries() is the
place where virtually all object writing occurs (object writing is
sometimes more of a concern for Forges than computation time), so
exiting early here isn't saving us much in object writes either
* the code changes needed to handle an earlier exit are slightly
more invasive in detect_and_process_renames() than for
process_entries().
Given the rareness of the even earlier conflicts, the limited savings
we'd get from exiting even earlier, and in an attempt to keep this
patch simpler, we don't guarantee that we actually exit on the first
conflict detected. We can always revisit this decision later if we
decide that a further micro-optimization to exit slightly earlier in
rare cases is worthwhile.
"avoid (not prevent) writing objects":
The detect_and_process_renames() call can also write objects to the
object store, when rename/rename conflicts involve one (or more) files
that have also been modified on both sides. Because of this alternate
call path leading to handle_content_merges(), our "early exit" does not
prevent writing objects entirely, even within the "outer layer"
(i.e. even within call_depth == 0). I figure that's fine though, since
we're already writing objects for the inner merges (i.e. for call_depth
> 0), which are likely going to represent vastly more objects than files
involved in rename/rename+modify/modify cases in the outer merge, on
average.
Signed-off-by: Elijah Newren <[email protected]>
|
This patch series was integrated into seen via git@b9b20b3. |
|
This patch series was integrated into seen via git@1887385. |
newren
changed the title
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@4be0180. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
newren
force-pushed
the
mergeability-only
branch
2 times, most recently
from
e771f7f to
f118243
Compare
|
This branch is now known as |
|
This patch series was integrated into seen via git@5f7e593. |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, "Kristoffer Haugsbakk" wrote (reply to this): On Wed, May 14, 2025, at 02:24, Elijah Newren via GitGitGadget wrote:
> Changes since v2:
>
> * Converted locations missed in v1 in changing --mergeability-only ->
> --dry-run
>
> Changes since v1:
>
> * Renamed --mergeability-only flag to --dry-run, as per suggestion from
> Junio
> * added some commit message clarifications
>
> This adds a new flag, --dry-run, to git merge-tree, which suppresses all
> output and leaves only the exit status (reflecting successful merge or
> conflict). This is useful for Git Forges in cases where they are only
> interested in whether two branches can be merged, without needing the actual
> merge result or conflict details.
>
> The advantage of the flag is two fold:
>
> * The merge machinery can exit once it detects a conflict, instead of
> continuing to compute merge result information
> * The merge machinery can avoid writing merged blobs and trees to the
> object store when in the outer layer of the merging process (more details
> in the first commit message).
>
> Elijah Newren (2):
> merge-ort: add a new mergeability_only option
> merge-tree: add a new --dry-run flag
All I can say is that this looks good considering the comments on v2.
Interdiff:
```
diff --git a/Documentation/git-merge-tree.adoc b/Documentation/git-merge-tree.adoc
index 7dcc1780619..74716b91019 100644
--- a/Documentation/git-merge-tree.adoc
+++ b/Documentation/git-merge-tree.adoc
@@ -65,11 +65,11 @@ OPTIONS
default is to include these messages if there are merge
conflicts, and to omit them otherwise.
---mergeability-only::
+--dry-run::
Disable all output from the program. Useful when you are only
interested in the exit status. Allows merge-tree to exit
- early on the first conflict it finds, and allows it to avoid
- writing most objects created by merges.
+ early when it finds a conflict, and allows it to avoid writing
+ most objects created by merges.
--allow-unrelated-histories::
merge-tree will by default error out if the two branches specified
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 579e81d5184..273ec171e98 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -596,13 +596,13 @@ int cmd_merge_tree(int argc,
if (dry_run && o.show_messages == -1)
o.show_messages = 0;
o.merge_options.mergeability_only = dry_run;
- die_for_incompatible_opt2(dry_run, "--mergeability-only",
+ die_for_incompatible_opt2(dry_run, "--dry-run",
o.show_messages, "--messages");
- die_for_incompatible_opt2(dry_run, "--mergeability-only",
+ die_for_incompatible_opt2(dry_run, "--dry-run",
o.name_only, "--name-only");
- die_for_incompatible_opt2(dry_run, "--mergeability-only",
+ die_for_incompatible_opt2(dry_run, "--dry-run",
o.use_stdin, "--stdin");
- die_for_incompatible_opt2(dry_run, "--mergeability-only",
+ die_for_incompatible_opt2(dry_run, "--dry-run",
!line_termination, "-z");
if (xopts.nr && o.mode == MODE_TRIVIAL)
``` |
|
This patch series was integrated into seen via git@991ef6a. |
|
This patch series was integrated into seen via git@f039320. |
|
User |
Git Forges may be interested in whether two branches can be merged while
not being interested in what the resulting merge tree is nor which files
conflicted. For such cases, add a new --quiet flag which
will make use of the new mergeability_only flag added to merge-ort in
the previous commit. This option allows the merge machinery to, in the
outer layer of the merge:
* exit early when a conflict is detected
* avoid writing (most) merged blobs/trees to the object store
Signed-off-by: Elijah Newren <[email protected]>
newren
changed the title
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@79990bf. |
|
There was a status update in the "Cooking" section about the branch "git merge-tree" learned an option to see if it resolves cleanly without actually creating a result. Will merge to 'next'. source: <[email protected]> |
| @@ -65,6 +65,12 @@ OPTIONS | |||
| default is to include these messages if there are merge | |||
There was a problem hiding this comment.
On the Git mailing list, "Kristoffer Haugsbakk" wrote (reply to this):
On Fri, May 16, 2025, at 22:04, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[email protected]>
> + if (quiet && o.show_messages == -1)
> + o.show_messages = 0;
> + o.merge_options.mergeability_only = quiet;
> + die_for_incompatible_opt2(quiet, "--quiet", o.show_messages, "--messages");
> + die_for_incompatible_opt2(quiet, "--quiet", o.name_only, "--name-only");
> + die_for_incompatible_opt2(quiet, "--quiet", o.use_stdin, "--stdin");
> + die_for_incompatible_opt2(quiet, "--quiet", !line_termination, "-z");
I’ve been using git-merge-tree(1) for some scripting but only today
tried out `--stdin` for printing refs that conflict.
```
# Pipe in pairs
merge_pairs=$(mktemp)
tee $merge_pairs \
| git merge-tree --stdin --no-messages \
| tr '\0' '\n' \
| grep --extended-regexp '^(1|0)$' \
| paste -d' ' - $merge_pairs \
| grep '^0' \
| cut -d' ' -f2-
```
(Previously I called the command in a loop)
I could imagine a `--format` option to just keep one of the arguments,
which means the tee(1) (for cross-referencing the ref) and all the other
things are gone:
```
git merge-tree --format='%(if)%(conflicted)%(then)oid2%(end)' --stdin
```
(But imagined options aside)
`--stdin` is presumably for efficiency and `--quiet`/`--dry-run`
definitely is. But `--quiet` can only be used in the mode where you can
only do a single merge, not in the `--stdin` batch mode.
`--quiet`/`--dry-run` with informational output (c.f. the above
die-for-incompatible) would “break” the documented output format since
conflicts haven’t been computed all the way and there are no OIDs for
successful merges. But the user is opting into a new mode here, never
seen before. Can’t they opt into a new informational mode where
`--stdin --quiet` can co-exist? Then you can have dry-run batch mode.
--
Kristoffer HaugsbakkThere was a problem hiding this comment.
On the Git mailing list, "Kristoffer Haugsbakk" wrote (reply to this):
On Sat, May 17, 2025, at 21:52, Kristoffer Haugsbakk wrote:
> Can’t they opt into a new informational mode where
> `--stdin --quiet` can co-exist?
Yes, I now immediately see the contradiction in the
literal text: “informational mode” and `--quiet`.
But I shall not weigh in on the naming matter.
--
Kristoffer Haugsbakk
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Elijah
On 16/05/2025 21:04, Elijah Newren via GitGitGadget wrote:
> Changes since v3:
> > * Renamed --dry-run -> --quiet . Any further naming suggestions?
Thanks for re-rolling this version looks good to me.
Thanks
Phillip
> Changes since v2:
> > * Converted locations missed in v1 in changing --mergeability-only ->
> --dry-run
> > Changes since v1:
> > * Renamed --mergeability-only flag to --dry-run, as per suggestion from
> Junio
> * added some commit message clarifications
> > This adds a new flag, --dry-run, to git merge-tree, which suppresses all
> output and leaves only the exit status (reflecting successful merge or
> conflict). This is useful for Git Forges in cases where they are only
> interested in whether two branches can be merged, without needing the actual
> merge result or conflict details.
> > The advantage of the flag is two fold:
> > * The merge machinery can exit once it detects a conflict, instead of
> continuing to compute merge result information
> * The merge machinery can avoid writing merged blobs and trees to the
> object store when in the outer layer of the merging process (more details
> in the first commit message).
> > Elijah Newren (2):
> merge-ort: add a new mergeability_only option
> merge-tree: add a new --quiet flag
> > Documentation/git-merge-tree.adoc | 6 +++++
> builtin/merge-tree.c | 18 +++++++++++++++
> merge-ort.c | 38 +++++++++++++++++++++++++------
> merge-ort.h | 1 +
> t/t4301-merge-tree-write-tree.sh | 38 +++++++++++++++++++++++++++++++
> 5 files changed, 94 insertions(+), 7 deletions(-)
> > > base-commit: 6c0bd1fc70efaf053abe4e57c976afdc72d15377
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1920%2Fnewren%2Fmergeability-only-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1920/newren/mergeability-only-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1920
> > Range-diff vs v3:
> > 1: 4757c4810d3 = 1: 4757c4810d3 merge-ort: add a new mergeability_only option
> 2: f11824317a8 ! 2: 7c40d3c9216 merge-tree: add a new --dry-run flag
> @@ Metadata
> Author: Elijah Newren <[email protected]>
> > ## Commit message ##
> - merge-tree: add a new --dry-run flag
> + merge-tree: add a new --quiet flag
> > Git Forges may be interested in whether two branches can be merged while
> not being interested in what the resulting merge tree is nor which files
> - conflicted. For such cases, add a new --dry-run flag which
> + conflicted. For such cases, add a new --quiet flag which
> will make use of the new mergeability_only flag added to merge-ort in
> the previous commit. This option allows the merge machinery to, in the
> outer layer of the merge:
> @@ Documentation/git-merge-tree.adoc: OPTIONS
> default is to include these messages if there are merge
> conflicts, and to omit them otherwise.
> > -+--dry-run::
> ++--quiet::
> + Disable all output from the program. Useful when you are only
> + interested in the exit status. Allows merge-tree to exit
> + early when it finds a conflict, and allows it to avoid writing
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc,
> int original_argc;
> const char *merge_base = NULL;
> int ret;
> -+ int dry_run = 0;
> ++ int quiet = 0;
> > const char * const merge_tree_usage[] = {
> N_("git merge-tree [--write-tree] [<options>] <branch1> <branch2>"),
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc,
> N_("do a trivial merge only"), MODE_TRIVIAL),
> OPT_BOOL(0, "messages", &o.show_messages,
> N_("also show informational/conflict messages")),
> -+ OPT_BOOL_F(0, "dry-run",
> -+ &dry_run,
> ++ OPT_BOOL_F(0, "quiet",
> ++ &quiet,
> + N_("suppress all output; only exit status wanted"),
> + PARSE_OPT_NONEG),
> OPT_SET_INT('z', NULL, &line_termination,
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc,
> argc = parse_options(argc, argv, prefix, mt_options,
> merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> > -+ if (dry_run && o.show_messages == -1)
> ++ if (quiet && o.show_messages == -1)
> + o.show_messages = 0;
> -+ o.merge_options.mergeability_only = dry_run;
> -+ die_for_incompatible_opt2(dry_run, "--dry-run",
> -+ o.show_messages, "--messages");
> -+ die_for_incompatible_opt2(dry_run, "--dry-run",
> -+ o.name_only, "--name-only");
> -+ die_for_incompatible_opt2(dry_run, "--dry-run",
> -+ o.use_stdin, "--stdin");
> -+ die_for_incompatible_opt2(dry_run, "--dry-run",
> -+ !line_termination, "-z");
> ++ o.merge_options.mergeability_only = quiet;
> ++ die_for_incompatible_opt2(quiet, "--quiet", o.show_messages, "--messages");
> ++ die_for_incompatible_opt2(quiet, "--quiet", o.name_only, "--name-only");
> ++ die_for_incompatible_opt2(quiet, "--quiet", o.use_stdin, "--stdin");
> ++ die_for_incompatible_opt2(quiet, "--quiet", !line_termination, "-z");
> +
> if (xopts.nr && o.mode == MODE_TRIVIAL)
> die(_("--trivial-merge is incompatible with all other options"));
> @@ t/t4301-merge-tree-write-tree.sh: test_expect_success setup '
> git commit -m first-commit
> '
> > -+test_expect_success '--dry-run on clean merge' '
> ++test_expect_success '--quiet on clean merge' '
> + # Get rid of loose objects to start with
> + git gc &&
> + echo "0 objects, 0 kilobytes" >expect &&
> @@ t/t4301-merge-tree-write-tree.sh: test_expect_success setup '
> + test_cmp expect actual &&
> +
> + # Ensure merge is successful (exit code of 0)
> -+ git merge-tree --write-tree --dry-run side1 side3 >output &&
> ++ git merge-tree --write-tree --quiet side1 side3 >output &&
> +
> + # Ensure there is no output
> + test_must_be_empty output &&
> @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Failed merge without rena
> grep "CONFLICT (modify/delete): numbers deleted" out
> '
> > -+test_expect_success '--dry-run on conflicted merge' '
> ++test_expect_success '--quiet on conflicted merge' '
> + # Get rid of loose objects to start with
> + git gc &&
> + echo "0 objects, 0 kilobytes" >expect &&
> @@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'Failed merge without rena
> + test_cmp expect actual &&
> +
> + # Ensure merge has conflict
> -+ test_expect_code 1 git merge-tree --write-tree --dry-run side1 side2 >output &&
> ++ test_expect_code 1 git merge-tree --write-tree --quiet side1 side2 >output &&
> +
> + # Ensure there is no output
> + test_must_be_empty output &&
> |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Phillip Wood <[email protected]> writes:
> Hi Elijah
>
> On 16/05/2025 21:04, Elijah Newren via GitGitGadget wrote:
>> Changes since v3:
>> * Renamed --dry-run -> --quiet . Any further naming suggestions?
>
> Thanks for re-rolling this version looks good to me.
>
> Thanks
>
> Phillip
Thanks, all. Let's mark it for 'next' and merge it down. |
|
This patch series was integrated into seen via git@0abdb88. |
|
This patch series was integrated into seen via git@e5022a6. |
|
This patch series was integrated into next via git@c3278b9. |
|
There was a status update in the "Cooking" section about the branch "git merge-tree" learned an option to see if it resolves cleanly without actually creating a result. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@fb06827. |
|
This patch series was integrated into seen via git@9fc2645. |
|
This patch series was integrated into seen via git@6fbfe8d. |
|
There was a status update in the "Cooking" section about the branch "git merge-tree" learned an option to see if it resolves cleanly without actually creating a result. Will merge to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@f545f40. |
|
This patch series was integrated into master via git@f545f40. |
|
This patch series was integrated into next via git@f545f40. |
|
Closed via f545f40. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes since v3:
Changes since v2:
Changes since v1:
This adds a new flag, --dry-run, to
git merge-tree, which suppresses all output and leaves only the exit status (reflecting successful merge or conflict). This is useful for Git Forges in cases where they are only interested in whether two branches can be merged, without needing the actual merge result or conflict details.The advantage of the flag is two fold:
cc: Elijah Newren [email protected]
cc: "Kristoffer Haugsbakk" [email protected]
cc: Phillip Wood [email protected]