* Re: [PATCH v4 15/19] object-file.c: release the "tag" in check_tag()
From: Jeff King @ 2023-01-18 18:39 UTC (permalink / raw)
To: René Scharfe
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Eric Sunshine
In-Reply-To: <fd883d86-0c85-6c72-a331-2e8b2064befe@web.de>
On Wed, Jan 18, 2023 at 07:05:32PM +0100, René Scharfe wrote:
> > 2. The point of this code is to find malformed input to hash-object.
> > We're probably better off feeding the buffer to fsck_commit(), etc.
> > It does more thorough checks, and these days it does not need an
> > object struct at all.
>
> I like the second one, as long as it won't check too much. c879daa237
> (Make hash-object more robust against malformed objects, 2011-02-05) added
> the checks that are now in object-file.c and intended to only validate the
> internal structure of objects, not relations between. It gave the example
> to allow adding a commit before its tree, which should be allowed. And
> IIUC fsck_object() fits that bill.
Yes, I think it will do what the right thing here. And having just
written up a quick series, the only tests which needed changes were ones
with syntactic problems. :)
I'll send it out in a few minutes.
> > Either of which would naturally fix the leak for tags. I'm not sure
> > there actually is a leak for commits, as commit structs don't store any
> > strings themselves.
>
> parse_commit_buffer() allocates the list of parents.
Yes, but it does so with lookup_commit(), so the resulting commit
objects are themselves reachable from the usual obj_hash, and thus not
leaked.
> Hmm, and it looks them up. Doesn't this violate the goal to allow
> dangling references?
No, because lookup_commit() is just about creating an in-process struct.
It doesn't look at the object database at all (though it would complain
if we had seen the same oid in-process as another type).
-Peff
^ permalink raw reply
* Re: [BUG?] 'git rebase --update-refs --whitespace=fix' incompatible, but not enforced
From: Philippe Blain @ 2023-01-18 18:07 UTC (permalink / raw)
To: Derrick Stolee, Elijah Newren; +Cc: Git mailing list, Derrick Stolee
In-Reply-To: <58a70c93-22ed-901c-44f2-3224734c27c3@github.com>
Le 2023-01-18 à 12:03, Derrick Stolee a écrit :
> On 1/18/2023 11:24 AM, Elijah Newren wrote:
>> On Wed, Jan 18, 2023 at 7:51 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
--- 8< ---
>> I have both changes over at
>> https://github.com/gitgitgadget/git/pull/1466; it doesn't include the
>> "--no-update-refs" hint, but maybe that's good enough? If so, I can
>> submit it. If not, do you want to alter or adopt some parts of my
>> patch and submit a v2?
>
> It sounds like you have a better handle on this and should take it
> from here. I look forward to your patch.
Thanks both for your quick answers and patches. I do think that the hint
about '--no-update-refs' could be valuable.
Cheers,
Philippe.
^ permalink raw reply
* Re: [PATCH v4 15/19] object-file.c: release the "tag" in check_tag()
From: René Scharfe @ 2023-01-18 18:05 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Eric Sunshine
In-Reply-To: <Y8gqJESD/wbEHZYb@coredump.intra.peff.net>
Am 18.01.23 um 18:19 schrieb Jeff King:
> On Tue, Jan 17, 2023 at 08:58:24PM +0100, René Scharfe wrote:
>
>> Am 17.01.23 um 18:11 schrieb Ævar Arnfjörð Bjarmason:
>>> Fix a memory leak that's been with us ever since c879daa2372 (Make
>>> hash-object more robust against malformed objects, 2011-02-05). With
>>> "HASH_FORMAT_CHECK" (used by "hash-object" and "replace") we'll parse
>>> tags into a throwaway variable on the stack, but weren't freeing the
>>> "item->tag" we might malloc() when doing so.
>>>
>>> The clearing that release_tag_memory() does for us is redundant here,
>>> but let's use it as-is anyway. It only has one other existing caller,
>>> which does need the tag to be cleared.
>>
>> Calling it is better than getting our hands dirty with tag internals
>> here.
>>
>> There's similar leak in check_commit() in the same file, but plugging it
>> would require exporting unparse_commit(). Or perhaps using the heavy
>> hammer that is release_commit_memory()? Anyway, it doesn't seem simple
>> to me, so that would be a patch for a separate series.
>
> I think both of these cases are a bit sketchy, because they create an
> object struct on the stack, and not via alloc_*_node(), which may
> violate assumptions elsewhere. In the case of the tag, it's probably OK.
> For the commit, though, the "index" field is going to be 0, which may
> confuse release_commit_memory(). It calls free_commit_buffer(), which is
> going to use that index to try to free from the buffer slab.
>
> So I'd be wary of calling that. I'm also slightly skeptical of the
> existing code that calls parse_commit_buffer(), but I think it works. We
> do not attach the buffer to the commit object there (good), and we pass
> "0" for check_graph, so the graph code (which IIRC also uses the index
> for some slab lookups) isn't run.
>
> I think this code would be better off either:
>
> 1. Just allocating an object struct in the usual way. I understand the
> desire not to spend extra memory, but parse_commit_buffer() is
> going to allocate structs under the hood for the tree and parent
> commits anyway.
>
> 2. The point of this code is to find malformed input to hash-object.
> We're probably better off feeding the buffer to fsck_commit(), etc.
> It does more thorough checks, and these days it does not need an
> object struct at all.
I like the second one, as long as it won't check too much. c879daa237
(Make hash-object more robust against malformed objects, 2011-02-05) added
the checks that are now in object-file.c and intended to only validate the
internal structure of objects, not relations between. It gave the example
to allow adding a commit before its tree, which should be allowed. And
IIUC fsck_object() fits that bill.
> Either of which would naturally fix the leak for tags. I'm not sure
> there actually is a leak for commits, as commit structs don't store any
> strings themselves.
parse_commit_buffer() allocates the list of parents.
Hmm, and it looks them up. Doesn't this violate the goal to allow
dangling references?
> I don't think any of that needs to hold up Ævar's current series,
> though. Fixing the leak this way in the meantime is OK, and then if we
> switch to one of the other techniques, the problem just goes away.
Right.
René
^ permalink raw reply
* Re: [PATCH v6 10/12] http: replace unsafe size_t multiplication with st_mult
From: Victoria Dye @ 2023-01-18 17:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason,
Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, M Hickford,
Jeff Hostetler, Glen Choo, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <230118.863588xeat.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
>
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Replace direct multiplication of two size_t parameters in curl response
>> stream handling callback functions with `st_mult` to guard against
>> overflows.
>>
>> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
>> ---
>> http.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/http.c b/http.c
>> index 8a5ba3f4776..a2a80318bb2 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -146,7 +146,7 @@ static int http_schannel_use_ssl_cainfo;
>>
>> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
>> {
>> - size_t size = eltsize * nmemb;
>> + size_t size = st_mult(eltsize, nmemb);
>> struct buffer *buffer = buffer_;
>>
>> if (size > buffer->buf.len - buffer->posn)
>> @@ -176,7 +176,7 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
>>
>> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
>> {
>> - size_t size = eltsize * nmemb;
>> + size_t size = st_mult(eltsize, nmemb);
>> struct strbuf *buffer = buffer_;
>>
>> strbuf_add(buffer, ptr, size);
>
> This is a really worthwhile fix, but shouldn't this be split into its
> own stand-alone patch? It applies on "master", and seems like something
> that's a good idea outside of this "test-http-server" topic.
While it's this change *can* stand alone, please keep in mind that
suggestions like this (recommending a series be split and resubmitted) can
be highly disruptive to the in-flight topic and the original contributor.
Monitoring and iterating on multiple series at once is time-consuming for
the contributor and reviewers, and often (although not in this case) it
creates a dependency of one series on another, which comes with a cost to
the maintainer's time. Not to say those recommendations should never be made
(e.g. in a clearly too-long series early in its review cycle, or when
certain patches lead to excessive context switching while reviewing), just
that they should be made more carefully, with consideration for the time of
other contributors.
So, with that in mind, I don't think this patch is critical enough to
separate into an independent submission, and (subjectively) it does not
disrupt the flow of this series.
^ permalink raw reply
* Re: [PATCH v4 15/19] object-file.c: release the "tag" in check_tag()
From: Jeff King @ 2023-01-18 17:19 UTC (permalink / raw)
To: René Scharfe
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Eric Sunshine
In-Reply-To: <7e571cdd-c0fa-7519-848c-b0bc4613abab@web.de>
On Tue, Jan 17, 2023 at 08:58:24PM +0100, René Scharfe wrote:
> Am 17.01.23 um 18:11 schrieb Ævar Arnfjörð Bjarmason:
> > Fix a memory leak that's been with us ever since c879daa2372 (Make
> > hash-object more robust against malformed objects, 2011-02-05). With
> > "HASH_FORMAT_CHECK" (used by "hash-object" and "replace") we'll parse
> > tags into a throwaway variable on the stack, but weren't freeing the
> > "item->tag" we might malloc() when doing so.
> >
> > The clearing that release_tag_memory() does for us is redundant here,
> > but let's use it as-is anyway. It only has one other existing caller,
> > which does need the tag to be cleared.
>
> Calling it is better than getting our hands dirty with tag internals
> here.
>
> There's similar leak in check_commit() in the same file, but plugging it
> would require exporting unparse_commit(). Or perhaps using the heavy
> hammer that is release_commit_memory()? Anyway, it doesn't seem simple
> to me, so that would be a patch for a separate series.
I think both of these cases are a bit sketchy, because they create an
object struct on the stack, and not via alloc_*_node(), which may
violate assumptions elsewhere. In the case of the tag, it's probably OK.
For the commit, though, the "index" field is going to be 0, which may
confuse release_commit_memory(). It calls free_commit_buffer(), which is
going to use that index to try to free from the buffer slab.
So I'd be wary of calling that. I'm also slightly skeptical of the
existing code that calls parse_commit_buffer(), but I think it works. We
do not attach the buffer to the commit object there (good), and we pass
"0" for check_graph, so the graph code (which IIRC also uses the index
for some slab lookups) isn't run.
I think this code would be better off either:
1. Just allocating an object struct in the usual way. I understand the
desire not to spend extra memory, but parse_commit_buffer() is
going to allocate structs under the hood for the tree and parent
commits anyway.
2. The point of this code is to find malformed input to hash-object.
We're probably better off feeding the buffer to fsck_commit(), etc.
It does more thorough checks, and these days it does not need an
object struct at all.
Either of which would naturally fix the leak for tags. I'm not sure
there actually is a leak for commits, as commit structs don't store any
strings themselves.
I don't think any of that needs to hold up Ævar's current series,
though. Fixing the leak this way in the meantime is OK, and then if we
switch to one of the other techniques, the problem just goes away.
-Peff
^ permalink raw reply
* Re: [BUG?] 'git rebase --update-refs --whitespace=fix' incompatible, but not enforced
From: Derrick Stolee @ 2023-01-18 17:03 UTC (permalink / raw)
To: Elijah Newren; +Cc: Philippe Blain, Git mailing list, Derrick Stolee
In-Reply-To: <CABPp-BGLVMoGiCeBMvyRhQmUSDEv8U7_U8=4B=Fh94=p_=QJVQ@mail.gmail.com>
On 1/18/2023 11:24 AM, Elijah Newren wrote:
> On Wed, Jan 18, 2023 at 7:51 AM Derrick Stolee <derrickstolee@github.com> wrote:
>> + /* Check for arguments that imply --apply before checking --apply itself. */
>> + if (options.update_refs) {
>> + const char *incompatible = NULL;
>> + if (options.git_am_opts.nr)
>> + incompatible = options.git_am_opts.v[0];
>> + else if (options.type == REBASE_APPLY)
>> + incompatible = "--apply";
>
> git_am_opts can include "-q" which is not incompatible since it's also
> supported under the merge backend.
True, but I don't think that happens at this point in time. Right now,
the only things in there should be those placed by the opts parsing.
Things like '-q' get translated later. However, it would be good to be
sure.
>> +
>> + if (incompatible) {
>> + int from_config = 0;
>> + if (!git_config_get_bool("rebase.updaterefs", &from_config) &&
>> + from_config) {
>> + warning(_("you have 'rebase.updateRefs' enabled in your config, "
>> + "but it is incompatible with one or more options;"));
>> + warning(_("run again with '--no-update-refs' to resolve the issue"));
>> + }
>> + die(_("options '%s' and '%s' cannot be used together"),
>> + "--upate-refs", incompatible);
>> + }
>> + }
>
> We already have imply_merge() to catch the range of incompatibilities;
> can we just use it?
That would be great! I didn't realize that.
>> +
>> if (trace2_is_enabled()) {
>> if (is_merge(&options))
>> trace2_cmd_mode("interactive");
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 462cefd25df..8681c8a07f8 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -2123,6 +2123,18 @@ test_expect_success '--update-refs: check failed ref update' '
>> test_cmp expect err.trimmed
>> '
>>
>> +test_expect_success '--apply options are incompatible with --update-refs' '
>> + for opt in "--whitespace=fix" "-C1" "--apply"
>> + do
>> + test_must_fail git rebase "$opt" --update-refs HEAD~1 2>err &&
>> + grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
>> +
>> + test_must_fail git -c rebase.updateRefs=true rebase "$opt" HEAD~1 2>err &&
>> + grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
>> + grep "you have '\''rebase.updateRefs'\'' enabled" err || return 1
>> + done
>> +'
>> +
>
> t3422 exists specifically for checking for incompatibilities of such
> options; the test should go over there.
>
> I have both changes over at
> https://github.com/gitgitgadget/git/pull/1466; it doesn't include the
> "--no-update-refs" hint, but maybe that's good enough? If so, I can
> submit it. If not, do you want to alter or adopt some parts of my
> patch and submit a v2?
It sounds like you have a better handle on this and should take it
from here. I look forward to your patch.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Luben Tuikov @ 2023-01-18 16:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Strawbridge, Michael, git@vger.kernel.org
In-Reply-To: <xmqqv8l34xkp.fsf@gitster.g>
On 2023-01-18 11:27, Junio C Hamano wrote:
> Luben Tuikov <luben.tuikov@amd.com> writes:
>
>> On 2023-01-17 02:31, Junio C Hamano wrote:
>>> Luben Tuikov <luben.tuikov@amd.com> writes:
>>>
>>>>> +test_expect_success $PREREQ "--validate hook supports header argument" '
>>>>> + write_script my-hooks/sendemail-validate <<-\EOF &&
>>>>> + if test -s "$2"
>>>>> + then
>>>>> + cat "$2" >actual
>>>>> + exit 1
>>>>> + fi
>>>>> + EOF
>>>
>>> If "$2" is not given, or an empty "$2" is given, is that an error?
>>> I am wondering if the lack of "else" clause (and the hook exits with
>>> success when "$2" is an empty file) here is intentional.
>>
>> I think we'll always have a $2, since it is the SMTP envelope and headers.
>
> We write our tests to verify _that_ assumption you have. A future
> developer mistakenly drops the code to append the file to the
> command line that invokes the hook, and we want our test to catch
> such a mistake.
>
> Do we really feed envelope? E.g. if the --envelope-sender=<who> is
> used, does $2 have the "From:" from the header and "MAIL TO" from
> the envelope separately?
I'm not sure--I thought we did, but yes, we should _test_ that we indeed
1) have/get $2, as a non-empty string,
2) it is a non-empty, readable file,
3) contains the test header we included in git-format-patch in the test.
This is what I meant when I wrote "we'll always have $2 ...", not having it
is failure of some kind and yes we should test for it.
--
Regards,
Luben
^ permalink raw reply
* [PATCH v8 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Michael Strawbridge @ 2023-01-18 16:32 UTC (permalink / raw)
To: git
Cc: michael.strawbridge, Luben Tuikov, Junio C Hamano,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20230118163203.488652-1-michael.strawbridge@amd.com>
To allow further flexibility in the Git hook, the SMTP header
information of the email which git-send-email intends to send, is now
passed as the 2nd argument to the sendemail-validate hook.
As an example, this can be useful for acting upon keywords in the
subject or specific email addresses.
As a consequence of needing all the header data, validation has been
moved later in the sequence to right before sending the emails instead
of at the beginning.
Cc: Luben Tuikov <luben.tuikov@amd.com>
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Acked-by: Luben Tuikov <luben.tuikov@amd.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
Documentation/githooks.txt | 27 ++++++++++++++---
git-send-email.perl | 60 ++++++++++++++++++++++----------------
t/t9001-send-email.sh | 27 +++++++++++++++--
3 files changed, 83 insertions(+), 31 deletions(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index a16e62bc8c..0decbfc92d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -583,10 +583,29 @@ processed by rebase.
sendemail-validate
~~~~~~~~~~~~~~~~~~
-This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,
-the name of the file that holds the e-mail to be sent. Exiting with a
-non-zero status causes `git send-email` to abort before sending any
-e-mails.
+This hook is invoked by linkgit:git-send-email[1].
+
+It takes these command line arguments. They are,
+1. the name of the file which holds the contents of the email to be sent.
+2. The name of the file which holds the SMTP headers of the email.
+
+The SMTP headers are passed in the exact same way as they are passed to the
+user's Mail Transport Agent (MTA). In effect, the email given to the user's
+MTA, is the contents of $2 followed by the contents of $1.
+
+Below is an example for a few common headers. Take notice of the
+capitalization and multi-line tab structure.
+
+ From: Example <from@example.com>
+ To: to@example.com
+ Cc: cc@example.com,
+ A <author@example.com>,
+ One <one@example.com>,
+ two@example.com
+ Subject: PATCH-STRING
+
+Exiting with a non-zero status causes `git send-email` to abort
+before sending any e-mails.
fsmonitor-watchman
~~~~~~~~~~~~~~~~~~
diff --git a/git-send-email.perl b/git-send-email.perl
index 42f135a266..d123dfd33d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -787,14 +787,6 @@ sub is_format_patch_arg {
@files = handle_backup_files(@files);
-if ($validate) {
- foreach my $f (@files) {
- unless (-p $f) {
- validate_patch($f, $target_xfer_encoding);
- }
- }
-}
-
if (@files) {
unless ($quiet) {
print $_,"\n" for (@files);
@@ -1124,6 +1116,30 @@ sub expand_one_alias {
our ($message_id, %mail, $subject, $in_reply_to, $references, $message,
$needs_confirm, $message_num, $ask_default);
+my ($message_id_stamp, $message_id_serial);
+
+$time = time - scalar $#files;
+
+if ($validate) {
+ foreach my $f (@files) {
+ unless (-p $f) {
+ pre_process_file($f, 1);
+
+ validate_patch($f, $target_xfer_encoding);
+ }
+ }
+}
+
+$in_reply_to = $initial_in_reply_to;
+$references = $initial_in_reply_to || '';
+$message_num = 0;
+
+foreach my $t (@files) {
+ while (!process_file($t)) {
+ # user edited the file
+ }
+}
+
sub extract_valid_address {
my $address = shift;
my $local_part_regexp = qr/[^<>"\s@]+/;
@@ -1189,7 +1205,6 @@ sub validate_address_list {
# We'll setup a template for the message id, using the "from" address:
-my ($message_id_stamp, $message_id_serial);
sub make_message_id {
my $uniq;
if (!defined $message_id_stamp) {
@@ -1214,10 +1229,6 @@ sub make_message_id {
#print "new message id = $message_id\n"; # Was useful for debugging
}
-
-
-$time = time - scalar $#files;
-
sub unquote_rfc2047 {
local ($_) = @_;
my $charset;
@@ -1738,10 +1749,6 @@ sub send_message {
return 1;
}
-$in_reply_to = $initial_in_reply_to;
-$references = $initial_in_reply_to || '';
-$message_num = 0;
-
sub pre_process_file {
my ($t, $quiet) = @_;
@@ -2006,12 +2013,6 @@ sub process_file {
return 1;
}
-foreach my $t (@files) {
- while (!process_file($t)) {
- # user edited the file
- }
-}
-
# Execute a command (e.g. $to_cmd) to get a list of email addresses
# and return a results array
sub recipients_cmd {
@@ -2101,11 +2102,20 @@ sub validate_patch {
chdir($repo->wc_path() or $repo->repo_path())
or die("chdir: $!");
local $ENV{"GIT_DIR"} = $repo->repo_path();
+
+ my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
+
+ require File::Temp;
+ my ($header_filehandle, $header_filename) = File::Temp::tempfile(
+ ".gitsendemail.header.XXXXXX", DIR => $repo->repo_path());
+ print $header_filehandle $header;
+
my @cmd = ("git", "hook", "run", "--ignore-missing",
$hook_name, "--");
- my @cmd_msg = (@cmd, "<patch>");
- my @cmd_run = (@cmd, $target);
+ my @cmd_msg = (@cmd, "<patch>", "<header>");
+ my @cmd_run = (@cmd, $target, $header_filename);
$hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
+ unlink($header_filehandle);
chdir($cwd_save) or die("chdir: $!");
}
if ($hook_error) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1130ef21b3..8a5c111a24 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -540,7 +540,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
test_path_is_file my-hooks.ran &&
cat >expect <<-EOF &&
fatal: longline.patch: rejected by sendemail-validate hook
- fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
+ fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
warning: no patches were sent
EOF
test_cmp expect actual
@@ -559,12 +559,35 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
test_path_is_file my-hooks.ran &&
cat >expect <<-EOF &&
fatal: longline.patch: rejected by sendemail-validate hook
- fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
+ fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
warning: no patches were sent
EOF
test_cmp expect actual
'
+test_expect_success $PREREQ "--validate hook supports header argument" '
+ write_script my-hooks/sendemail-validate <<-\EOF &&
+ if test "$#" -ge 2
+ then
+ grep "X-test-header: v1.0" "$2"
+ else
+ echo "No header arg passed"
+ exit 1
+ fi
+ EOF
+ test_config core.hooksPath "my-hooks" &&
+ rm -fr outdir &&
+ git format-patch \
+ --add-header="X-test-header: v1.0" \
+ -n HEAD^1 -o outdir &&
+ git send-email \
+ --dry-run \
+ --to=nobody@example.com \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ --validate \
+ outdir/000?-*.patch
+'
+
for enc in 7bit 8bit quoted-printable base64
do
test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '
--
2.34.1
^ permalink raw reply related
* [PATCH v8 1/2] send-email: refactor header generation functions
From: Michael Strawbridge @ 2023-01-18 16:32 UTC (permalink / raw)
To: git
Cc: michael.strawbridge, Luben Tuikov, Junio C Hamano,
Ævar Arnfjörð Bjarmason
In-Reply-To: <20230118163203.488652-1-michael.strawbridge@amd.com>
Split process_file and send_message into easier to use functions.
Making SMTP header information widely available.
Cc: Luben Tuikov <luben.tuikov@amd.com>
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Acked-by: Luben Tuikov <luben.tuikov@amd.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 49 ++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 18 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 5861e99a6e..42f135a266 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1495,16 +1495,7 @@ sub file_name_is_absolute {
return File::Spec::Functions::file_name_is_absolute($path);
}
-# Prepares the email, then asks the user what to do.
-#
-# If the user chooses to send the email, it's sent and 1 is returned.
-# If the user chooses not to send the email, 0 is returned.
-# If the user decides they want to make further edits, -1 is returned and the
-# caller is expected to call send_message again after the edits are performed.
-#
-# If an error occurs sending the email, this just dies.
-
-sub send_message {
+sub gen_header {
my @recipients = unique_email_list(@to);
@cc = (grep { my $cc = extract_valid_address_or_die($_);
not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
@@ -1546,6 +1537,22 @@ sub send_message {
if (@xh) {
$header .= join("\n", @xh) . "\n";
}
+ my $recipients_ref = \@recipients;
+ return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header);
+}
+
+# Prepares the email, then asks the user what to do.
+#
+# If the user chooses to send the email, it's sent and 1 is returned.
+# If the user chooses not to send the email, 0 is returned.
+# If the user decides they want to make further edits, -1 is returned and the
+# caller is expected to call send_message again after the edits are performed.
+#
+# If an error occurs sending the email, this just dies.
+
+sub send_message {
+ my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
+ my @recipients = @$recipients_ref;
my @sendmail_parameters = ('-i', @recipients);
my $raw_from = $sender;
@@ -1735,11 +1742,8 @@ sub send_message {
$references = $initial_in_reply_to || '';
$message_num = 0;
-# Prepares the email, prompts the user, sends it out
-# Returns 0 if an edit was done and the function should be called again, or 1
-# otherwise.
-sub process_file {
- my ($t) = @_;
+sub pre_process_file {
+ my ($t, $quiet) = @_;
open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
@@ -1893,9 +1897,9 @@ sub process_file {
}
close $fh;
- push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t)
+ push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t, $quiet)
if defined $to_cmd;
- push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t)
+ push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t, $quiet)
if defined $cc_cmd && !$suppress_cc{'cccmd'};
if ($broken_encoding{$t} && !$has_content_type) {
@@ -1954,6 +1958,15 @@ sub process_file {
@initial_to = @to;
}
}
+}
+
+# Prepares the email, prompts the user, and sends it out
+# Returns 0 if an edit was done and the function should be called again, or 1
+# on the email being successfully sent out.
+sub process_file {
+ my ($t) = @_;
+
+ pre_process_file($t, $quiet);
my $message_was_sent = send_message();
if ($message_was_sent == -1) {
@@ -2002,7 +2015,7 @@ sub process_file {
# Execute a command (e.g. $to_cmd) to get a list of email addresses
# and return a results array
sub recipients_cmd {
- my ($prefix, $what, $cmd, $file) = @_;
+ my ($prefix, $what, $cmd, $file, $quiet) = @_;
my @addresses = ();
open my $fh, "-|", "$cmd \Q$file\E"
--
2.34.1
^ permalink raw reply related
* [PATCH v8 0/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Michael Strawbridge @ 2023-01-18 16:32 UTC (permalink / raw)
To: git; +Cc: michael.strawbridge
This patch version addresses:
- extra test for number of arguments in the header t9001 test
- removed use of envelope in the documentation
- re-arranged the validation loop and surrounding code to be closer to
the other code not in functions
- made a note in the commit message that the moving of validation later
can have some difference in stdout compared to before but actual email
functionality should be ok
Hopefully this looks better.
Michael Strawbridge (2):
send-email: refactor header generation functions
send-email: expose header information to git-send-email's
sendemail-validate hook
Documentation/githooks.txt | 27 +++++++--
git-send-email.perl | 109 ++++++++++++++++++++++---------------
t/t9001-send-email.sh | 27 ++++++++-
3 files changed, 114 insertions(+), 49 deletions(-)
--
2.34.1
^ permalink raw reply
* Re: [PATCH] ssh signing: better error message when key not in agent
From: Phillip Wood @ 2023-01-18 16:29 UTC (permalink / raw)
To: Adam Szkoda; +Cc: Adam Szkoda via GitGitGadget, git, Fabian Stelzer
In-Reply-To: <CAEroKagqxC86X0SD8=tK0w+yXL7QecZ+z_7sja-K6ajs0=Z=BQ@mail.gmail.com>
Hi Adam
I've cc'd Fabian who knows more about the ssh signing code that I do.
On 18/01/2023 15:28, Adam Szkoda wrote:
> Hi Phillip,
>
> Good point! My first thought is to try doing a stat() syscall on the
> path from 'user.signingKey' to see if it exists and if not, treat it
> as a public key (and pass the -U option). If that sounds reasonable,
> I can update the patch.
My reading of the documentation is that user.signingKey may point to a
public or private key so I'm not sure how stat()ing would help. Looking
at the code in sign_buffer_ssh() we have a function is_literal_ssh_key()
that checks if the config value is a public key. When the user passes
the path to a key we could read the file check use is_literal_ssh_key()
to check if it is a public key (or possibly just check if the file
begins with "ssh-"). Fabian - does that sound reasonable?
Best Wishes
Phillip
> Best
> — Adam
>
>
> On Wed, Jan 18, 2023 at 3:34 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 18/01/2023 11:10, Phillip Wood wrote:
>>>> the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All
>>>> that
>>>> needs to be done is to pass an additional backward-compatible option
>>>> -U to
>>>> 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets
>>>> the file
>>>> as public key and expects to find the private key in the agent.
>>>
>>> The documentation for user.signingKey says
>>>
>>> If gpg.format is set to ssh this can contain the path to either your
>>> private ssh key or the public key when ssh-agent is used.
>>>
>>> If I've understood correctly passing -U will prevent users from setting
>>> this to a private key.
>>
>> If there is an easy way to tell if the user has given us a public key
>> then we could pass "-U" in that case.
>>
>> Best Wishes
>>
>> Phillip
^ permalink raw reply
* Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Junio C Hamano @ 2023-01-18 16:27 UTC (permalink / raw)
To: Luben Tuikov; +Cc: Strawbridge, Michael, git@vger.kernel.org
In-Reply-To: <71623e1d-805d-cdc7-d872-224821c1383c@amd.com>
Luben Tuikov <luben.tuikov@amd.com> writes:
> On 2023-01-17 02:31, Junio C Hamano wrote:
>> Luben Tuikov <luben.tuikov@amd.com> writes:
>>
>>>> +test_expect_success $PREREQ "--validate hook supports header argument" '
>>>> + write_script my-hooks/sendemail-validate <<-\EOF &&
>>>> + if test -s "$2"
>>>> + then
>>>> + cat "$2" >actual
>>>> + exit 1
>>>> + fi
>>>> + EOF
>>
>> If "$2" is not given, or an empty "$2" is given, is that an error?
>> I am wondering if the lack of "else" clause (and the hook exits with
>> success when "$2" is an empty file) here is intentional.
>
> I think we'll always have a $2, since it is the SMTP envelope and headers.
We write our tests to verify _that_ assumption you have. A future
developer mistakenly drops the code to append the file to the
command line that invokes the hook, and we want our test to catch
such a mistake.
Do we really feed envelope? E.g. if the --envelope-sender=<who> is
used, does $2 have the "From:" from the header and "MAIL TO" from
the envelope separately?
^ permalink raw reply
* Re: [BUG?] 'git rebase --update-refs --whitespace=fix' incompatible, but not enforced
From: Elijah Newren @ 2023-01-18 16:24 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Philippe Blain, Git mailing list, Derrick Stolee
In-Reply-To: <9445830b-d172-c3b6-ef60-ae4931cab84b@github.com>
On Wed, Jan 18, 2023 at 7:51 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 1/17/2023 5:02 PM, Philippe Blain wrote:
> > Hi Elijah and Stolee,
> >
> > First, Stolee, thanks a lot for adding '--update-refs', it is very useful (I've
> > added it to my config so I don't forget to use it).
>
> I'm glad you're using it, and thanks for the report!
>
> > I recently learned that 'git rebase --whitespace=fix' exists, which is also
> > great but since it uses the apply backend, it can't be used with --update-refs.
> > I understand this, and the fact that adding '--whitespace=fix' to the merge
> > backend would be a big task; this is not what this email is about.
>
> I also use --whitespace=fix, and am disappointed that it doesn't work with
> --update-refs. I guess I just haven't used it in my workflows that depend on
> --update-refs.
>
> > I think there is a bug in that the code does not enforce the fact that
> > both options can't be used together. Whenever '--whitespace' or '-C' are used,
> > the code defaults to the apply backend:
> ...
> > but 'is_merge' only checks if 'opts->type == REBASE_MERGE', so the check only
> > works if --merge was given explicitely, but not when none of '--merge' or '--apply'
> > were given (and so the default "merge" backend is used).
> >
> > I would have expected the code to die telling me --update-refs and --whitespace
> > are incompatible. But instead it defaulted to --apply, and (of course) did not
> > update the refs in my history (which I found out later).
>
> Yes, I agree that there should be an error message (and a die(), not just a
> warning). I quickly whipped up the patch below, which should resolve your
> concern.
>
> Note that I was a bit worried about users with the config option and not
> just those that specify the option over the command-line. I put in an extra
> warning for those users, but I could see the community wanting different
> behavior there.
>
> Let me know what you think. If we need a new version, I'll create a new
> thread for that review.
I was just about to post a patch as well --
https://github.com/gitgitgadget/git/pull/1466.
> --- >8 ---
> From fe310b37796b0b15554481eb1cfa3942a9200b4b Mon Sep 17 00:00:00 2001
> From: Derrick Stolee <derrickstolee@github.com>
> Date: Wed, 18 Jan 2023 10:38:18 -0500
> Subject: [PATCH] rebase: die for both --apply and --update-refs
>
> The --apply backend is not compatible with the --update-refs option,
> which requires the interactive backend. Without any warning, users
> running 'git rebase --whitespace=fix' with --update-refs (or
> rebase.updateRefs=true in their config) will realize that their non-HEAD
> branches did not come along for the ride.
>
> Fix this with a die() message in the presence of both options. Since we
> cannot distinguish between the --update-refs option and the config
> option at this point, do an extra check to see if --update-refs was
> implied by the config and present a helpful warning to use
> --no-update-refs.
>
> It is possible that users might want to be able to use options such as
> --whitespace=fix with rebase.updateRefs=true in their config without
> explicitly adding --no-update-refs. However, it is probably safest to
> require them to explicitly opt-in to that behavior. Users with the
> config option likely expect that their refs will be automatically
> updated and this will help warn them that the action they are doing is
> likely destructive to their branch organization.
>
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> builtin/rebase.c | 21 +++++++++++++++++++++
> t/t3404-rebase-interactive.sh | 12 ++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1481c5b6a5b..5330258c865 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1247,6 +1247,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> die(_("The --edit-todo action can only be used during "
> "interactive rebase."));
>
> + /* Check for arguments that imply --apply before checking --apply itself. */
> + if (options.update_refs) {
> + const char *incompatible = NULL;
> + if (options.git_am_opts.nr)
> + incompatible = options.git_am_opts.v[0];
> + else if (options.type == REBASE_APPLY)
> + incompatible = "--apply";
git_am_opts can include "-q" which is not incompatible since it's also
supported under the merge backend.
> +
> + if (incompatible) {
> + int from_config = 0;
> + if (!git_config_get_bool("rebase.updaterefs", &from_config) &&
> + from_config) {
> + warning(_("you have 'rebase.updateRefs' enabled in your config, "
> + "but it is incompatible with one or more options;"));
> + warning(_("run again with '--no-update-refs' to resolve the issue"));
> + }
> + die(_("options '%s' and '%s' cannot be used together"),
> + "--upate-refs", incompatible);
> + }
> + }
We already have imply_merge() to catch the range of incompatibilities;
can we just use it?
> +
> if (trace2_is_enabled()) {
> if (is_merge(&options))
> trace2_cmd_mode("interactive");
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 462cefd25df..8681c8a07f8 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -2123,6 +2123,18 @@ test_expect_success '--update-refs: check failed ref update' '
> test_cmp expect err.trimmed
> '
>
> +test_expect_success '--apply options are incompatible with --update-refs' '
> + for opt in "--whitespace=fix" "-C1" "--apply"
> + do
> + test_must_fail git rebase "$opt" --update-refs HEAD~1 2>err &&
> + grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
> +
> + test_must_fail git -c rebase.updateRefs=true rebase "$opt" HEAD~1 2>err &&
> + grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
> + grep "you have '\''rebase.updateRefs'\'' enabled" err || return 1
> + done
> +'
> +
t3422 exists specifically for checking for incompatibilities of such
options; the test should go over there.
I have both changes over at
https://github.com/gitgitgadget/git/pull/1466; it doesn't include the
"--no-update-refs" hint, but maybe that's good enough? If so, I can
submit it. If not, do you want to alter or adopt some parts of my
patch and submit a v2?
^ permalink raw reply
* Re: [PATCH] git-cat-file.txt: fix list continuations rendering literally
From: Junio C Hamano @ 2023-01-18 16:23 UTC (permalink / raw)
To: Martin Ågren; +Cc: git, Siddharth Asthana
In-Reply-To: <20230118082749.1252459-1-martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
> With Asciidoctor, all of the '+' introduced in a797c0ea04 ("cat-file:
> add mailmap support to --batch-check option", 2022-12-20) render
> literally rather than functioning as list continuations. With asciidoc,
> this renders just fine.
Good to have somebody who is careful to notice these differences ;-)
> For consistency, let's wrap all three of these inner lists from
> a797c0ea04 in open blocks. This also future-proofs us a little -- if we
> ever gain more contents after any of those first two lists, as we did
> already in a797c0ea04 for the third list, we're prepared and should
> render fine with both asciidoc and Asciidoctor from the start.
Nice.
Are you comparing both roff output and html output, by the way?
Thanks.
^ permalink raw reply
* Re: [PATCH v3] grep: correctly identify utf-8 characters with \{b,w} in -P
From: Junio C Hamano @ 2023-01-18 16:20 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Carlo Arenas, git, Diomidis Spinellis
In-Reply-To: <230118.86tu0ovyvj.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> GNU grep -P has no knob and would likely never have one.
>
> I think the general knob in not just GNU grep but GNU utils and the
> wider *nix landscape is "tweak your LC_ALL and/or other locale
> varibales".
>
> Which works for it, and will work for us once we're using PCRE2_UCP too.
>
>> So for now, I think we should acknowledge the bug, provide an option
>> for people that might need the fix, and fix all other problems we
>> have, which will include changes in PCRE2 as well to better fit our
>> use case.
>
> Hrm, what are those PCRE2 changes? The one I saw so far (or was it a
> proposal) was to just make its "grep" utility use the PCRE2_UCP like GNU
> grep is now doing in its unreleased version in its git repo...
Yeah, I didn't understand Carlo's comment in that paragraph at all.
In short, it sounds to me that the earlier one that added PCRE2_UCP
unconditionally would be the best alternative among those that have
been discussed.
^ permalink raw reply
* Re: [BUG?] 'git rebase --update-refs --whitespace=fix' incompatible, but not enforced
From: Phillip Wood @ 2023-01-18 16:16 UTC (permalink / raw)
To: Derrick Stolee, Philippe Blain, Git mailing list, Derrick Stolee,
Elijah Newren
In-Reply-To: <9445830b-d172-c3b6-ef60-ae4931cab84b@github.com>
Hi Stolee
On 18/01/2023 15:51, Derrick Stolee wrote:
> On 1/17/2023 5:02 PM, Philippe Blain wrote:
> Subject: [PATCH] rebase: die for both --apply and --update-refs
>
> The --apply backend is not compatible with the --update-refs option,
> which requires the interactive backend. Without any warning, users
> running 'git rebase --whitespace=fix' with --update-refs (or
> rebase.updateRefs=true in their config) will realize that their non-HEAD
> branches did not come along for the ride.
I think that for other options such as "--exec" that require the "merge"
backend we call imply_merge() and then have some code that will error
out if there are also options that require the "apply" backend. Is it
possible to do that here?
> Fix this with a die() message in the presence of both options. Since we
> cannot distinguish between the --update-refs option and the config
> option at this point, do an extra check to see if --update-refs was
> implied by the config and present a helpful warning to use
> --no-update-refs.
>
> It is possible that users might want to be able to use options such as
> --whitespace=fix with rebase.updateRefs=true in their config without
> explicitly adding --no-update-refs. However, it is probably safest to
> require them to explicitly opt-in to that behavior. Users with the
> config option likely expect that their refs will be automatically
> updated and this will help warn them that the action they are doing is
> likely destructive to their branch organization.
I agree that requiring an explicit --no-update-refs in that case is best.
Best Wishes
Phillip
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> builtin/rebase.c | 21 +++++++++++++++++++++
> t/t3404-rebase-interactive.sh | 12 ++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1481c5b6a5b..5330258c865 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1247,6 +1247,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> die(_("The --edit-todo action can only be used during "
> "interactive rebase."));
>
> + /* Check for arguments that imply --apply before checking --apply itself. */
> + if (options.update_refs) {
> + const char *incompatible = NULL;
> + if (options.git_am_opts.nr)
> + incompatible = options.git_am_opts.v[0];
> + else if (options.type == REBASE_APPLY)
> + incompatible = "--apply";
> +
> + if (incompatible) {
> + int from_config = 0;
> + if (!git_config_get_bool("rebase.updaterefs", &from_config) &&
> + from_config) {
> + warning(_("you have 'rebase.updateRefs' enabled in your config, "
> + "but it is incompatible with one or more options;"));
> + warning(_("run again with '--no-update-refs' to resolve the issue"));
> + }
> + die(_("options '%s' and '%s' cannot be used together"),
> + "--upate-refs", incompatible);
> + }
> + }
> +
> if (trace2_is_enabled()) {
> if (is_merge(&options))
> trace2_cmd_mode("interactive");
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 462cefd25df..8681c8a07f8 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -2123,6 +2123,18 @@ test_expect_success '--update-refs: check failed ref update' '
> test_cmp expect err.trimmed
> '
>
> +test_expect_success '--apply options are incompatible with --update-refs' '
> + for opt in "--whitespace=fix" "-C1" "--apply"
> + do
> + test_must_fail git rebase "$opt" --update-refs HEAD~1 2>err &&
> + grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
> +
> + test_must_fail git -c rebase.updateRefs=true rebase "$opt" HEAD~1 2>err &&
> + grep "options '\''--upate-refs'\'' and '\''$opt'\'' cannot be used together" err &&
> + grep "you have '\''rebase.updateRefs'\'' enabled" err || return 1
> + done
> +'
> +
> # This must be the last test in this file
> test_expect_success '$EDITOR and friends are unchanged' '
> test_editor_unchanged
^ permalink raw reply
* Re: [PATCH v4 08/19] worktree: fix a trivial leak in prune_worktrees()
From: Junio C Hamano @ 2023-01-18 16:16 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, René Scharfe, Eric Sunshine
In-Reply-To: <230118.86o7qwxg4e.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> diff --git c/builtin/branch.c w/builtin/branch.c
>> index f63fd45edb..4fe7757670 100644
>> --- c/builtin/branch.c
>> +++ w/builtin/branch.c
>> @@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>> if (filter.abbrev == -1)
>> filter.abbrev = DEFAULT_ABBREV;
>> filter.ignore_case = icase;
>> + UNLEAK(filter);
>>
>> finalize_colopts(&colopts, -1);
>> if (filter.verbose) {
>
> I'll send a v5 re-roll without this change, sorry.
I'd rather see your reroll with the above addition of UNLEAK() than
without it, to fix the breakage.
^ permalink raw reply
* Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees
From: Junio C Hamano @ 2023-01-18 16:10 UTC (permalink / raw)
To: Carlo Arenas; +Cc: git, pclouds, Jinwook Jeong, Rubén Justo, Eric Sunshine
In-Reply-To: <CAPUEspjmFJUjZmNBH=f_-TY3hYnOtgVjBY-YtWuo15eg3a+5cQ@mail.gmail.com>
Carlo Arenas <carenas@gmail.com> writes:
> the following is the way to override:
>
> $ git checkout --ignore-other-worktrees -B foo
My points were that (1) the option is way too unintuitive to
anybody, other than those who stared at the implementation of the
problematic logic for too long, and that (2) it wasn't mentioned in
the proposed log message or documentation updates. If "--force" is
made to mean that, it might be easier to discover to the users, but
I have no strong opinion on that (meaning: there may be downsides to
allow use of "--force" to override this safety, given that "-B" is
already considered as a "forcing" version of "-b").
> `git branch` has no flag and will die even when `-f` is used
If "--force" does not force it, I suspect it should be considered a
bug.
Thanks.
^ permalink raw reply
* [PATCH v3 5/8] builtin/rebase.c: fix "options.onto_name" leak
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
Similar to the existing "squash_onto_name" added in [1] we need to
free() the xstrdup()'d "options.onto.name" added for "--keep-base" in
[2]..
1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/rebase.c | 4 +++-
t/t3416-rebase-onto-threedots.sh | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5859a5387d8..5c474fb6edd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1037,6 +1037,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
struct string_list strategy_options = STRING_LIST_INIT_NODUP;
struct object_id squash_onto;
char *squash_onto_name = NULL;
+ char *keep_base_onto_name = NULL;
int reschedule_failed_exec = -1;
int allow_preemptive_ff = 1;
int preserve_merges_selected = 0;
@@ -1660,7 +1661,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
strbuf_addstr(&buf, options.upstream_name);
strbuf_addstr(&buf, "...");
strbuf_addstr(&buf, branch_name);
- options.onto_name = xstrdup(buf.buf);
+ options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
} else if (!options.onto_name)
options.onto_name = options.upstream_name;
if (strstr(options.onto_name, "...")) {
@@ -1836,6 +1837,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
free(options.strategy);
strbuf_release(&options.git_format_patch_opt);
free(squash_onto_name);
+ free(keep_base_onto_name);
string_list_clear(&exec, 0);
string_list_clear(&strategy_options, 0);
return !!ret;
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index ea501f2b42b..f8c4ed78c9e 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY/lib-rebase.sh"
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v3 7/8] builtin/rebase.c: free() "options.strategy_opts"
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
When the "strategy_opts" member was added in ba1905a5fef (builtin
rebase: add support for custom merge strategies, 2018-09-04) the
corresponding free() for it at the end of cmd_rebase() wasn't added,
let's do so.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/rebase.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5c474fb6edd..611c1d20ddf 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1835,6 +1835,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
free(options.gpg_sign_opt);
free(options.cmd);
free(options.strategy);
+ free(options.strategy_opts);
strbuf_release(&options.git_format_patch_opt);
free(squash_onto_name);
free(keep_base_onto_name);
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v3 8/8] commit.c: free() revs.commit in get_fork_point()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
Fix a memory leak that's been with us since d96855ff517 (merge-base:
teach "--fork-point" mode, 2013-10-23).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
commit.c | 1 +
t/t3431-rebase-fork-point.sh | 1 +
t/t3432-rebase-fast-forward.sh | 1 +
3 files changed, 3 insertions(+)
diff --git a/commit.c b/commit.c
index 14538a811ae..e433c33bb01 100644
--- a/commit.c
+++ b/commit.c
@@ -1033,6 +1033,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)
ret = bases->item;
cleanup_return:
+ free(revs.commit);
free_commit_list(bases);
free(full_refname);
return ret;
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 70e81363569..4bfc779bb87 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -8,6 +8,7 @@ test_description='git rebase --fork-point test'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# A---B---D---E (main)
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 5086e14c022..7f1a5dd3deb 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -8,6 +8,7 @@ test_description='ensure rebase fast-forwards commits when possible'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v3 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
In [1] the strbuf_release(&msgbuf) was moved into this
do_pick_commit(), but didn't take into account the case of [2], where
we'd return before the strbuf_release(&msgbuf).
Then when the "fixup" support was added in [3] this leak got worse, as
in this error case we added another place where we'd "return" before
reaching the strbuf_release().
This changes the behavior so that we'll call
update_abort_safety_file() in these cases where we'd previously
"return", but as noted in [4] "update_abort_safety_file() is a no-op
when rebasing and you're changing code that is only run when
rebasing.". Here "no-op" refers to the early return in
update_abort_safety_file() if git_path_seq_dir() doesn't exist.
1. 452202c74b8 (sequencer: stop releasing the strbuf in
write_message(), 2016-10-21)
2. f241ff0d0a9 (prepare the builtins for a libified merge_recursive(),
2016-07-26)
3. 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and
'squash' commands, 2017-01-02)
4. https://lore.kernel.org/git/bcace50b-a4c3-c468-94a3-4fe0c62b3671@dunelm.org.uk/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
sequencer.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index ea568d99498..2c5ed5bc5ea 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2277,8 +2277,10 @@ static int do_pick_commit(struct repository *r,
reword = 1;
else if (is_fixup(command)) {
if (update_squash_messages(r, command, commit,
- opts, item->flags))
- return -1;
+ opts, item->flags)) {
+ res = -1;
+ goto leave;
+ }
flags |= AMEND_MSG;
if (!final_fixup)
msg_file = rebase_path_squash_msg();
@@ -2288,9 +2290,11 @@ static int do_pick_commit(struct repository *r,
} else {
const char *dest = git_path_squash_msg(r);
unlink(dest);
- if (copy_file(dest, rebase_path_squash_msg(), 0666))
- return error(_("could not rename '%s' to '%s'"),
- rebase_path_squash_msg(), dest);
+ if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
+ res = error(_("could not rename '%s' to '%s'"),
+ rebase_path_squash_msg(), dest);
+ goto leave;
+ }
unlink(git_path_merge_msg(r));
msg_file = dest;
flags |= EDIT_MSG;
@@ -2328,7 +2332,6 @@ static int do_pick_commit(struct repository *r,
free_commit_list(common);
free_commit_list(remotes);
}
- strbuf_release(&msgbuf);
/*
* If the merge was clean or if it failed due to conflict, we write
@@ -2402,6 +2405,7 @@ static int do_pick_commit(struct repository *r,
leave:
free_message(commit, &msg);
free(author);
+ strbuf_release(&msgbuf);
update_abort_safety_file();
return res;
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v3 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
In [1] and [2] I added the code being moved here to cmd_revert() and
cmd_cherry_pick(), now that we've got a "replay_opts_release()" for
the "struct replay_opts" it should know how to free these "revs",
rather than having these users reach into the struct to free its
individual members.
1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
2022-11-08)
2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/revert.c | 6 ------
sequencer.c | 3 +++
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 1cab16bf3ed..77d2035616e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -248,9 +248,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
res = run_sequencer(argc, argv, &opts);
if (res < 0)
die(_("revert failed"));
- if (opts.revs)
- release_revisions(opts.revs);
- free(opts.revs);
replay_opts_release(&opts);
return res;
}
@@ -263,9 +260,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
opts.action = REPLAY_PICK;
sequencer_init_config(&opts);
res = run_sequencer(argc, argv, &opts);
- if (opts.revs)
- release_revisions(opts.revs);
- free(opts.revs);
if (res < 0)
die(_("cherry-pick failed"));
replay_opts_release(&opts);
diff --git a/sequencer.c b/sequencer.c
index 8ff29262c1e..ea568d99498 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -361,6 +361,9 @@ void replay_opts_release(struct replay_opts *opts)
free(opts->xopts[i]);
free(opts->xopts);
strbuf_release(&opts->current_fixups);
+ if (opts->revs)
+ release_revisions(opts->revs);
+ free(opts->revs);
}
int sequencer_remove_state(struct replay_opts *opts)
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v3 3/8] rebase & sequencer API: fix get_replay_opts() leak in "rebase"
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
Make the recently added replay_opts_release() function non-static and
use it for freeing the "struct replay_opts" constructed by the
get_replay_opts() function in "builtin/rebase.c". See [1] for the
initial addition of get_replay_opts().
To safely call our new replay_opts_release() we'll need to stop
calling it in sequencer_remove_state(), and instead call it where we
allocate the "struct replay_opts" itself.
This is because in e.g. do_interactive_rebase() we construct a "struct
replay_opts" with "get_replay_opts()", and then call
"complete_action()". If we get far enough in that function without
encountering errors we'll call "pick_commits()" which (indirectly)
calls sequencer_remove_state() at the end.
But if we encounter errors anywhere along the way we'd punt out early,
and not free() the memory we allocated. Remembering whether we
previously called sequencer_remove_state() would be a hassle.
Using a FREE_AND_NULL() pattern would also work, as it would be safe
replay_opts_release() repeatedly, but let's fix this properly instead,
by having the owner of the data free() it.
See [2] for the initial implementation of "sequencer_remove_state()",
which assumed that it should be removing the full (including on-disk)
rebase state as a one-off.
1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
2019-04-17)
2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
2011-08-04)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/rebase.c | 4 ++++
builtin/revert.c | 2 ++
sequencer.c | 4 +---
sequencer.h | 1 +
t/t3405-rebase-malformed.sh | 1 +
t/t3412-rebase-root.sh | 1 +
t/t3419-rebase-patch-id.sh | 1 +
t/t3423-rebase-reword.sh | 1 +
t/t3425-rebase-topology-merges.sh | 2 ++
t/t3437-rebase-fixup-options.sh | 1 +
t/t3438-rebase-broken-files.sh | 2 ++
t/t3501-revert-cherry-pick.sh | 1 +
t/t3502-cherry-pick-merge.sh | 1 +
t/t3503-cherry-pick-root.sh | 1 +
t/t3506-cherry-pick-ff.sh | 1 +
t/t3511-cherry-pick-x.sh | 1 +
t/t7402-submodule-rebase.sh | 1 +
t/t9106-git-svn-commit-diff-clobber.sh | 1 -
t/t9164-git-svn-dcommit-concurrent.sh | 1 -
19 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7141fd5e0c1..5859a5387d8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -301,6 +301,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
}
cleanup:
+ replay_opts_release(&replay);
string_list_clear(&commands, 0);
free(revisions);
free(shortrevisions);
@@ -343,6 +344,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
struct replay_opts replay_opts = get_replay_opts(opts);
ret = sequencer_continue(the_repository, &replay_opts);
+ replay_opts_release(&replay_opts);
break;
}
case ACTION_EDIT_TODO:
@@ -558,6 +560,7 @@ static int finish_rebase(struct rebase_options *opts)
replay.action = REPLAY_INTERACTIVE_REBASE;
ret = sequencer_remove_state(&replay);
+ replay_opts_release(&replay);
} else {
strbuf_addstr(&dir, opts->state_dir);
if (remove_dir_recursively(&dir, 0))
@@ -1331,6 +1334,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
replay.action = REPLAY_INTERACTIVE_REBASE;
ret = sequencer_remove_state(&replay);
+ replay_opts_release(&replay);
} else {
strbuf_reset(&buf);
strbuf_addstr(&buf, options.state_dir);
diff --git a/builtin/revert.c b/builtin/revert.c
index f2d86d2a8f9..1cab16bf3ed 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -251,6 +251,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
if (opts.revs)
release_revisions(opts.revs);
free(opts.revs);
+ replay_opts_release(&opts);
return res;
}
@@ -267,5 +268,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
free(opts.revs);
if (res < 0)
die(_("cherry-pick failed"));
+ replay_opts_release(&opts);
return res;
}
diff --git a/sequencer.c b/sequencer.c
index d385bea2bed..8ff29262c1e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,7 +351,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
return buf.buf;
}
-static void replay_opts_release(struct replay_opts *opts)
+void replay_opts_release(struct replay_opts *opts)
{
free(opts->gpg_sign);
free(opts->reflog_action);
@@ -385,8 +385,6 @@ int sequencer_remove_state(struct replay_opts *opts)
}
}
- replay_opts_release(opts);
-
strbuf_reset(&buf);
strbuf_addstr(&buf, get_dir(opts));
if (remove_dir_recursively(&buf, 0))
diff --git a/sequencer.h b/sequencer.h
index 888c18aad71..3bcdfa1b586 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
int sequencer_continue(struct repository *repo, struct replay_opts *opts);
int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
int sequencer_skip(struct repository *repo, struct replay_opts *opts);
+void replay_opts_release(struct replay_opts *opts);
int sequencer_remove_state(struct replay_opts *opts);
#define TODO_LIST_KEEP_EMPTY (1U << 0)
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
index 25243318618..8979bc34073 100755
--- a/t/t3405-rebase-malformed.sh
+++ b/t/t3405-rebase-malformed.sh
@@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 58371d8a547..e75b3d0e07c 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
log_with_names () {
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 7181f176b81..6c61f240cf9 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -5,6 +5,7 @@ test_description='git rebase - test patch id computation'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
scramble () {
diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
index 4859bb8f722..2fab703d615 100755
--- a/t/t3423-rebase-reword.sh
+++ b/t/t3423-rebase-reword.sh
@@ -2,6 +2,7 @@
test_description='git rebase interactive with rewording'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
index 63acc1ea4da..a16428bdf54 100755
--- a/t/t3425-rebase-topology-merges.sh
+++ b/t/t3425-rebase-topology-merges.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='rebase topology tests with merges'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index c023fefd681..274699dadb8 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
"amend!" upon --autosquash.
'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
index b92a3ce46b8..c614c4f2e4b 100755
--- a/t/t3438-rebase-broken-files.sh
+++ b/t/t3438-rebase-broken-files.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='rebase behavior when on-disk files are broken'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'set up conflicting branches' '
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 1f4cfc37449..2f3e3e24169 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -13,6 +13,7 @@ test_description='test cherry-pick and revert with renames
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 5495eacfec1..1b2c0d6aca6 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -11,6 +11,7 @@ test_description='cherry picking and reverting a merge
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index 95fe4feaeee..76d393dc8a3 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -5,6 +5,7 @@ test_description='test cherry-picking (and reverting) a root commit'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index 7e11bd4a4c5..b71bad17b85 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -5,6 +5,7 @@ test_description='test cherry-picking with --ff option'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 84a587daf3a..dd5d92ef302 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -2,6 +2,7 @@
test_description='Test cherry-pick -x and -s'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
pristine_detach () {
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index ebeca12a711..b19792b3269 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -5,6 +5,7 @@
test_description='Test rebasing, stashing, etc. with submodules'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index 3cab0b9720a..bca496c40e0 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -3,7 +3,6 @@
# Copyright (c) 2006 Eric Wong
test_description='git svn commit-diff clobber'
-TEST_FAILS_SANITIZE_LEAK=true
. ./lib-git-svn.sh
test_expect_success 'initialize repo' '
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index 1465156072e..c8e6c0733f4 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -5,7 +5,6 @@
test_description='concurrent git svn dcommit'
-TEST_FAILS_SANITIZE_LEAK=true
. ./lib-git-svn.sh
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v3 2/8] sequencer.c: split up sequencer_remove_state()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
Split off the free()-ing in sequencer_remove_state() into a utility
function, which will be adjusted and called independent of the other
code in sequencer_remove_state() in a subsequent commit.
The only functional change here is changing the "int" to a "size_t",
which is the correct type, as "xopts_nr" is a "size_t".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
sequencer.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index bcb662e23be..d385bea2bed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,10 +351,22 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
return buf.buf;
}
+static void replay_opts_release(struct replay_opts *opts)
+{
+ free(opts->gpg_sign);
+ free(opts->reflog_action);
+ free(opts->default_strategy);
+ free(opts->strategy);
+ for (size_t i = 0; i < opts->xopts_nr; i++)
+ free(opts->xopts[i]);
+ free(opts->xopts);
+ strbuf_release(&opts->current_fixups);
+}
+
int sequencer_remove_state(struct replay_opts *opts)
{
struct strbuf buf = STRBUF_INIT;
- int i, ret = 0;
+ int ret = 0;
if (is_rebase_i(opts) &&
strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
@@ -373,14 +385,7 @@ int sequencer_remove_state(struct replay_opts *opts)
}
}
- free(opts->gpg_sign);
- free(opts->reflog_action);
- free(opts->default_strategy);
- free(opts->strategy);
- for (i = 0; i < opts->xopts_nr; i++)
- free(opts->xopts[i]);
- free(opts->xopts);
- strbuf_release(&opts->current_fixups);
+ replay_opts_release(opts);
strbuf_reset(&buf);
strbuf_addstr(&buf, get_dir(opts));
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox