From: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>, Taylor Blau <me@ttaylorr.com>,
Johannes Schindelin <johannes.schindelin@gmx.de>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH v2] merge-tree: fix segmentation fault in read-only repositories
Date: Wed, 21 Sep 2022 22:08:03 +0000 [thread overview]
Message-ID: <pull.1362.v2.git.1663798083240.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1362.git.1663774248660.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Independent of the question whether we want `git merge-tree` to report
the tree name even when it failed to write the tree objects in a
read-only repository, there is no question that we should avoid a
segmentation fault.
And when we report an invalid tree name (because the tree could not be
written), we need the exit status to be non-zero.
Let's make it so.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge-tree: fix segmentation fault in read-only repositories
Turns out that the segmentation fault reported by Taylor
[https://lore.kernel.org/git/YyopQD+LvPucnz3w@nand.local/] happened
while testing merge-ort in a read-only repository, and that the upstream
version of git merge-tree is as affected as GitHub's internal version.
Note: I briefly considered using the OID of the_hash_algo->empty_tree
instead of null_oid() when no tree object could be constructed. However,
I have come to the conclusion that this could potentially cause its own
set of problems because it would relate to a valid tree object even if
we do not have any valid tree object to play with.
Also note: The question I hinted at in the commit message, namely
whether or not to try harder to construct a tree object even if we
cannot write it out, maybe merits a longer discussion, one that I think
we should have after v2.38.0 is released, so as not to distract from
focusing on v2.38.0.
Changes since v1:
* Using the SANITY prereq now
* If the merge was aborted due to a write error, exit with a non-zero
code even if the (potentially partial) merge is clean
* The test case now also verifies that the git merge-tree command fails
in a read-only repository even if the merge would have succeeded
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1362%2Fdscho%2Fmerge-tree-in-read-only-repos-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1362/dscho/merge-tree-in-read-only-repos-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1362
Range-diff vs v1:
1: beba3fd9a95 ! 1: 1de4df3f471 merge-tree: fix segmentation fault in read-only repositories
@@ Metadata
## Commit message ##
merge-tree: fix segmentation fault in read-only repositories
- Independent of the question whether we want `git merge-tree` to report a
- tree name even when it failed to write the tree objects in a read-only
- repository, there is no question that we should avoid a segmentation
- fault.
+ Independent of the question whether we want `git merge-tree` to report
+ the tree name even when it failed to write the tree objects in a
+ read-only repository, there is no question that we should avoid a
+ segmentation fault.
+
+ And when we report an invalid tree name (because the tree could not be
+ written), we need the exit status to be non-zero.
+
+ Let's make it so.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
if (!result.clean) {
struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
const char *last = NULL;
+@@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
+ &result);
+ }
+ merge_finalize(&opt, &result);
+- return !result.clean; /* result.clean < 0 handled above */
++ return !result.tree || !result.clean; /* result.clean < 0 handled above */
+ }
+
+ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
## t/t4301-merge-tree-write-tree.sh ##
@@ t/t4301-merge-tree-write-tree.sh: test_expect_success 'can override merge of unrelated histories' '
test_cmp expect actual
'
-+test_expect_success 'merge-ort fails gracefully in a read-only repository' '
++test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '
+ git init --bare read-only &&
-+ git push read-only side1 side2 &&
++ git push read-only side1 side2 side3 &&
+ test_when_finished "chmod -R u+w read-only" &&
+ chmod -R a-w read-only &&
++ test_must_fail git -C read-only merge-tree side1 side3 &&
+ test_must_fail git -C read-only merge-tree side1 side2
+'
+
builtin/merge-tree.c | 6 ++++--
t/t4301-merge-tree-write-tree.sh | 9 +++++++++
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index ae5782917b9..0df24eb82d4 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -412,6 +412,7 @@ static int real_merge(struct merge_tree_options *o,
struct commit_list *merge_bases = NULL;
struct merge_options opt;
struct merge_result result = { 0 };
+ const struct object_id *tree_oid;
parent1 = get_merge_parent(branch1);
if (!parent1)
@@ -446,7 +447,8 @@ static int real_merge(struct merge_tree_options *o,
if (o->show_messages == -1)
o->show_messages = !result.clean;
- printf("%s%c", oid_to_hex(&result.tree->object.oid), line_termination);
+ tree_oid = result.tree ? &result.tree->object.oid : null_oid();
+ printf("%s%c", oid_to_hex(tree_oid), line_termination);
if (!result.clean) {
struct string_list conflicted_files = STRING_LIST_INIT_NODUP;
const char *last = NULL;
@@ -473,7 +475,7 @@ static int real_merge(struct merge_tree_options *o,
&result);
}
merge_finalize(&opt, &result);
- return !result.clean; /* result.clean < 0 handled above */
+ return !result.tree || !result.clean; /* result.clean < 0 handled above */
}
int cmd_merge_tree(int argc, const char **argv, const char *prefix)
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 28ca5c38bb5..013b77144bd 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -810,4 +810,13 @@ test_expect_success 'can override merge of unrelated histories' '
test_cmp expect actual
'
+test_expect_success SANITY 'merge-ort fails gracefully in a read-only repository' '
+ git init --bare read-only &&
+ git push read-only side1 side2 side3 &&
+ test_when_finished "chmod -R u+w read-only" &&
+ chmod -R a-w read-only &&
+ test_must_fail git -C read-only merge-tree side1 side3 &&
+ test_must_fail git -C read-only merge-tree side1 side2
+'
+
test_done
base-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14
--
gitgitgadget
next prev parent reply other threads:[~2022-09-21 22:08 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-21 15:30 [PATCH] merge-tree: fix segmentation fault in read-only repositories Johannes Schindelin via GitGitGadget
2022-09-21 15:42 ` Taylor Blau
2022-09-21 20:12 ` Johannes Schindelin
2022-09-21 18:12 ` Junio C Hamano
2022-09-21 20:13 ` Johannes Schindelin
2022-09-21 22:08 ` Johannes Schindelin via GitGitGadget [this message]
2022-09-21 22:24 ` [PATCH v2] " Junio C Hamano
2022-09-22 19:52 ` Johannes Schindelin
2022-09-21 22:56 ` Elijah Newren
2022-09-21 23:11 ` Junio C Hamano
2022-09-22 17:24 ` Johannes Schindelin
2022-09-22 19:50 ` Johannes Schindelin
2022-09-23 1:47 ` Elijah Newren
2022-09-22 19:46 ` [PATCH v3] merge-ort: " Johannes Schindelin via GitGitGadget
2022-09-23 1:38 ` Elijah Newren
2022-09-23 9:55 ` Johannes Schindelin
2022-09-26 21:55 ` [PATCH v4 0/2] merge-tree: " Johannes Schindelin via GitGitGadget
2022-09-26 21:55 ` [PATCH v4 1/2] merge-ort: " Johannes Schindelin via GitGitGadget
2022-09-26 21:55 ` [PATCH v4 2/2] merge-ort: return early when failing to write a blob Johannes Schindelin via GitGitGadget
2022-09-26 22:07 ` Junio C Hamano
2022-09-27 8:05 ` Elijah Newren
2022-09-27 16:45 ` Junio C Hamano
2022-09-27 8:11 ` [PATCH v4 0/2] merge-tree: fix segmentation fault in read-only repositories Elijah Newren
2022-09-28 7:29 ` [PATCH v5 " Johannes Schindelin via GitGitGadget
2022-09-28 7:29 ` [PATCH v5 1/2] merge-ort: " Johannes Schindelin via GitGitGadget
2022-09-28 7:29 ` [PATCH v5 2/2] merge-ort: return early when failing to write a blob Johannes Schindelin via GitGitGadget
2022-09-28 15:53 ` Junio C Hamano
2022-09-29 1:36 ` Elijah Newren
2022-09-29 1:49 ` Junio C Hamano
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=pull.1362.v2.git.1663798083240.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=johannes.schindelin@gmx.de \
--cc=me@ttaylorr.com \
--cc=newren@gmail.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).