* Re: [PATCH v4 1/7] bulk-checkin: extract abstract `bulk_checkin_source`
From: Junio C Hamano @ 2023-10-20 16:55 UTC (permalink / raw)
To: Jeff King
Cc: Taylor Blau, git, Elijah Newren, Eric W. Biederman,
Patrick Steinhardt
In-Reply-To: <20231020073500.GE1642714@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Oct 19, 2023 at 01:28:42PM -0400, Taylor Blau wrote:
>
>> +struct bulk_checkin_source {
>> + enum { SOURCE_FILE } type;
>> +
>> + /* SOURCE_FILE fields */
>> + int fd;
>> +
>> + /* common fields */
>> + size_t size;
>> + const char *path;
>> +};
>
> This is a pretty minor nit, but we may find that "SOURCE_FILE" is not
> sufficiently name-spaced. Enum tags are in the global namespace, so
> the compiler will barf if there are any conflicts.
A very good point. This was one of the reasons why I suggested
avoiding the switch() based on a new enum altogether and use a
vtable based approach instead. But it may do while this one is
private to the file and never exposed to other subsystems.
> It might be OK here, since this is local to a single C file (so we at
> least are not hurting other code), but we may be in trouble if code in a
> header file is less careful. There is already a near-miss here with
> GREP_SOURCE_FILE, but fortunately grep.h is indeed careful. :)
>
> (I notice that ref-filter.c similarly uses SOURCE_* for an internal
> enum).
Thanks.
^ permalink raw reply
* Re: [PATCH v4 0/7] merge-ort: implement support for packing objects together
From: Junio C Hamano @ 2023-10-20 16:53 UTC (permalink / raw)
To: Jeff King
Cc: Taylor Blau, git, Elijah Newren, Eric W. Biederman,
Patrick Steinhardt
In-Reply-To: <20231020072941.GD1642714@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> - Having not worked with the bulk-checkin code much before, I thought
> it only put in one blob per pack, and thus you'd have to teach it to
> handle multiple objects, too. But fortunately I was wrong, and it
> handles this case already. (I mention this mainly because we had
> talked about it off-list a few weeks ago, and some of my confusion
> may have seeped into my comments then).
You probably had it mixed up with a real limitation, i.e., "unlike
fast-import that considers the immediate previous object could be a
good delta base, this code does not even attempt any deltification"
;-)
> - I did briefly wonder if we need this SOURCE abstraction at all. The
> file source assumes we can seek() and read(), so it must be a
> regular file. We could probably just mmap() it and treat it as
> in-core, too (this is what index_fd() and index_path() do under the
> hood!).
Ahhhh, I've never thought of that one. I actually was hoping that
we can add an abstraction layer that can be reused elsewhere and
everywhere (not limited to the bulk-checkin codepath for its source
material), which might help folks who want to refactor Git enough to
allow them plug $CORP specific goodies like virtual filesystem layer
;-).
> But it would probably be a disservice to any platforms that
> do not support mmap, as the fallback is to read into a full buffer
> (and the whole point of the bulk checkin code is to avoid that).
Yes. The result was a pleasant read.
Thanks, both.
^ permalink raw reply
* Re: [PATCH] typo: fix the typo 'neeed' into 'needed' in the comment under merge-ort.c
From: Junio C Hamano @ 2023-10-20 16:44 UTC (permalink / raw)
To: 王常新; +Cc: git
In-Reply-To: <42D0A6FD-1314-4977-92E6-BF6535D82E8A@gmail.com>
王常新 <wchangxin824@gmail.com> writes:
> It is my official name. But the email address is not a valid one. Should I rewrite the commit message?
We try to keep the name and address on Signed-off-by: the official
one that we can give court if/when some copyright troll sues us (see
Documentation/SubmittingPatches:sign-off), and one of them (if more
than one developers signed off the patch) must match the primary
author's name and address.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option
From: Junio C Hamano @ 2023-10-20 16:41 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps
In-Reply-To: <xmqqttqmtcc2.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Rather, I was wondering if we need to use object flags to mark these
> objects, or can do what we want to do without using any object flags
> at all. For the purpose of reporting "missing" objects, wouldn't it
> be sufficient to walk the object graph and report our findings as we
> go? To avoid reporting the same object twice, as we reasonably can
> expect that the missing objects are minority (compared to the total
> number of objects), perhaps the codepath that makes such a report
> can use a hashmap of object_ids or something, for example.
Digging from the bottom,
* builtin/rev-list.c:show_commit() gets "struct rev_list_info *"
that has "struct rev_info *" [*].
* list-objects.c:do_traverse() calls revision.c:get_revision() to
obtain commits, some of which may be missing ones, and things
behind get_revision() are responsible for marking the commit as
missing. It has "struct traversal_context *", among whose
members is the "revs" member that is the "struct rev_info *".
* revision.c:get_revision() and machinery behind it ultimately
discovers a missing commit in the revision.c:process_parents()
that loops over the parents commit_list. It of course has access
to "struct rev_info *".
So, presumably, if we add a new member to "struct rev_info" that
optionally [*] points at an oidset that records the object names of
missing objects we discovered so far (i.e., the set of missing
objects), the location we set the MISSING bit of a commit can
instead add the object name of the commit to the set. And we can
export a function that takes "struct rev_info *" and "struct object
*" (or "struct object_id *") to check for membership in the "set of
missing objects", which would be used where we checked the MISSING
bit of a commit.
I do not know the performance implications of going this route, but
if we do not find a suitable vacant bit, we do not have to use any
object flags bit to do this, if we go this route, I would think. I
may be missing some details that breaks the above outline, though.
[Footnotes]
* A potential #leftoverbits tangent.
Why is "rev_list_info" structure declared in <bisect.h>? I
suspect that this is a fallout from recent header file shuffling,
but given who uses it (among which is rev-list:show_commit() that
has very little to do with bisection and uses the information in
rev_list_info when doing its normal non-bisect things), it does
not make much sense.
* When .do_not_die_on_missing_objects is false, it can and should
be left NULL, but presumably we use the "do not die" bit even
when we are not necessarily collecting the missing objects? So
the new member cannot replace the "do not die" bit completely.
^ permalink raw reply
* [PATCH v2] grep: die gracefully when outside repository
From: Kristoffer Haugsbakk @ 2023-10-20 16:40 UTC (permalink / raw)
To: code; +Cc: gitster, ks1322, git
In-Reply-To: <087c92e3904dd774f672373727c300bf7f5f6369.1697317276.git.code@khaugsbakk.name>
Die gracefully when `git grep --no-index` is run outside of a Git
repository and the path is outside the directory tree.
If you are not in a Git repository and say:
git grep --no-index search ..
You trigger a `BUG`:
BUG: environment.c:213: git environment hasn't been setup
Aborted (core dumped)
Because `..` is a valid path which is treated as a pathspec. Then
`pathspec` figures out that it is not in the current directory tree. The
`BUG` is triggered when `pathspec` tries to advice the user about how the
path is not in the current (non-existing) repository.
Reported-by: ks1322 ks1322 <ks1322@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
- Initialize `hint_path` after we know that we are in a Git repository
- Apply Junio's suggestion for the test: https://lore.kernel.org/git/xmqqzg0hf0g8.fsf@gitster.g/
pathspec.c | 7 ++++++-
t/t7810-grep.sh | 27 +++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/pathspec.c b/pathspec.c
index 3a3a5724c44..264b4929a55 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -467,7 +467,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
match = prefix_path_gently(prefix, prefixlen,
&prefixlen, copyfrom);
if (!match) {
- const char *hint_path = get_git_work_tree();
+ const char *hint_path;
+
+ if (!have_git_dir())
+ die(_("'%s' is outside the directory tree"),
+ copyfrom);
+ hint_path = get_git_work_tree();
if (!hint_path)
hint_path = get_git_dir();
die(_("%s: '%s' is outside repository at '%s'"), elt,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 39d6d713ecb..84838c0fe1b 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -1234,6 +1234,33 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
)
'
+test_expect_success 'no repository with path outside $cwd' '
+ test_when_finished rm -fr non &&
+ rm -fr non &&
+ mkdir -p non/git/sub non/tig &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search .. 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../tig 2>error &&
+ grep "is outside the directory tree" error
+ ) &&
+ (
+ GIT_CEILING_DIRECTORIES="$(pwd)/non" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd non/git &&
+ test_expect_code 128 git grep --no-index search ../non 2>error &&
+ grep "no such path in the working tree" error
+ )
+'
+
test_expect_success 'inside git repository but with --no-index' '
rm -fr is &&
mkdir -p is/git/sub &&
--
2.42.0.2.g879ad04204
^ permalink raw reply related
* Re: Thanks and One Suggestion
From: Emily Shaffer @ 2023-10-20 16:08 UTC (permalink / raw)
To: Brock Whitaker, Git List
In-Reply-To: <1D0EE77C-6DEF-4509-9846-6897C73C7CDD@gmail.com>
Adding back the list - first I accidentally HTML'd my reply, and then
Brock replied directly to me :) In general, please make sure to keep
the mailing list CC'd on responses, Brock. (And turn off HTML in your
MUA. Don't be like me :) )
I agree looking in my own browser that the monospace - both inline and
blockquote - is a little difficult to read. Take
https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---amend
for an example of both. However, I'll leave it up to the git-scm folks
to recommend a fix.
- Emily
On Fri, Oct 20, 2023 at 8:13 AM Brock Whitaker
<brock.e.whitaker@gmail.com> wrote:
>
> The code examples are the hardest to read. My monitor probably isn’t helping.
>
> Brock
>
> On Oct 20, 2023, at 10:56 AM, Emily Shaffer <nasamuffin@google.com> wrote:
>
>
> Hi Brock, thanks for your feedback. Could you specify which orange-on-white is difficult for you? There are a few different examples I see at a quick glance - headings, fixed-width text, sidebar. Which is bothering you?
>
> - Emily
>
> On Fri, Oct 20, 2023 at 7:20 AM Brock Whitaker <brock.e.whitaker@gmail.com> wrote:
>>
>> Thank you for the git-scm site. It's an invaluable resource I use often.
>>
>> My only issue with it is the text isn't as easy to read as I would
>> like. The orange text on a white background is a little tough to read
>> sometimes. A higher contrast between text and background color would
>> help.
>>
>> Splitting hairs,
>>
>> B
>>
>> PS - Thanks again for a super useful resource!
>>
^ permalink raw reply
* Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-20 14:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <CAOLa=ZR4Qd9x-zVv9OtL7KTU00W2uT-kxdhKOLFZbi5cnHdiSQ@mail.gmail.com>
On Fri, Oct 20, 2023 at 1:14 PM Karthik Nayak <karthik.188@gmail.com> wrote:
> I'm not entirely sure what the best path to take here to be honest. I will look
> and see if there are any bits where overlapping doesn't cause any issues
> in the meantime.
Was trying to use bit number 12, which coincides METAINFO_SHOWN in
builtin/blame.c.
From skimming over the code, METAINFO_SHOWN is used only within
blame.c and there
should not be collisions here since blame.c doesn't set the
do_not_die_on_missing_objects bit
either.
The tests seem to pass here too [1]. This could be the potential
solution here, Junio, what do you think?
I could send another version if this approach looks good.
[1] https://gitlab.com/gitlab-org/git/-/jobs/5339132966
Diff against the tip of v3:
diff --git a/object.h b/object.h
index b76830fce1..0cb8c02b95 100644
--- a/object.h
+++ b/object.h
@@ -62,7 +62,7 @@ void object_array_init(struct object_array *array);
/*
* object flag allocation:
- * revision.h: 0---------10 15 22------28
+ * revision.h: 0---------10 12 15 22------27
* fetch-pack.c: 01 67
* negotiator/default.c: 2--5
* walker.c: 0-2
@@ -82,7 +82,7 @@ void object_array_init(struct object_array *array);
* builtin/show-branch.c: 0-------------------------------------------26
* builtin/unpack-objects.c: 2021
*/
-#define FLAG_BITS 29
+#define FLAG_BITS 28
#define TYPE_BITS 3
diff --git a/revision.h b/revision.h
index d3c1ca0f4e..d2de9d65e4 100644
--- a/revision.h
+++ b/revision.h
@@ -38,6 +38,9 @@
#define PATCHSAME (1u<<9)
#define BOTTOM (1u<<10)
+/* WARNING: This is also used as METAINFO_SHOWN in builtin/blame.c. */
+#define MISSING (1u<<12)
+
/* WARNING: This is also used as REACHABLE in commit-graph.c. */
#define PULL_MERGE (1u<<15)
@@ -53,7 +56,6 @@
#define ANCESTRY_PATH (1u<<27)
#define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN |
TRACK_LINEAR | PULL_MERGE)
-#define MISSING (1u<<28)
#define DECORATE_SHORT_REFS 1
#define DECORATE_FULL_REFS 2
^ permalink raw reply related
* Thanks and One Suggestion
From: Brock Whitaker @ 2023-10-20 14:20 UTC (permalink / raw)
To: git
Thank you for the git-scm site. It's an invaluable resource I use often.
My only issue with it is the text isn't as easy to read as I would
like. The orange text on a white background is a little tough to read
sometimes. A higher contrast between text and background color would
help.
Splitting hairs,
B
PS - Thanks again for a super useful resource!
^ permalink raw reply
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Achu Luma @ 2023-10-20 13:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <CAFR+8Dwxr3iV+R7een0t2sYXUWu1XHhQcLVuqMhOsSg9Bt4wrg@mail.gmail.com>
Dear Git Community and Mentors,
I hope this email finds you well. As a follow-up to my previous
application, I'd like to provide additional details on the process of
migrating existing unit tests from the t/helper/ directory to the new
Git unit test framework.
-- Identify Target Unit Tests: Start by identifying the specific unit
tests in the t/helper/ directory that we want to port to the new Git
unit test framework. Ensure that the tests are suitable for migration
and that the benefits of doing so outweigh the effort(By avoiding
integration tests). The following points have been developed with on
going work on the unit-tests framework visible here: 1-
https://lore.kernel.org/git/0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com/
2- https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc
-- Create a New C Test File: For each unit test I plan to migrate,
create a new C source file (.c) in the Git project's test suite
directory(t/unit-tests). Name it appropriately to reflect the purpose
of the test.
-- Include Necessary Headers:In the new C test file, include the
necessary Git unit test framework headers. Typically, this includes
headers like "test-lib.h" and others relevant to the specific test.
#include "test-lib.h"
-- Convert Test Logic: Refactor the test logic from the original Shell
script into the new C-based test format. Use the testing macros
provided by the Git unit test framework, such as test_expect_success,
test_expect_failure, etc., to define the tests.
test_expect_success("simple progress display", "{
// Test logic here...
}");
-- Add Test Descriptions: Provide clear and informative descriptions
for each test using the testing macros. These descriptions will help
in identifying the purpose of each test when the test suite is run.
-- Define a Test Entry Point: Create a cmd_main function as the entry
point for the C-based tests. Inside this function, include the test
functions using the testing macros.
int cmd_main(int argc, const char **argv) {
// Test functions...
return test_done();
}
-- Ensure TAP Format Output: Ensure that the C-based tests produce
output in the Test Anything Protocol (TAP) format. This format
includes the test name, status (ok or not ok), and any diagnostic
information.
-- Test Interaction: Ensure that the migrated tests interact correctly
with the new Git unit test framework and any other tests that may be
relevant. Consider dependencies and interactions with other parts of
the Git project.
-- Test Execution: Run the migrated tests to verify that they produce
the expected results when executed as part of the Git project's test
suite. Use the Git testing framework's test runners to execute the
tests.
-- Documentation Update: Update the Git project's documentation to
reflect the changes made during the migration. Include a reference to
the original unit tests in the t/helper/ directory and indicate that
these tests have been ported to the new Git unit test framework.
By following these points, I think I can successfully port existing
unit tests from the t/helper/ directory to use the new Git unit test
framework. This migration helps standardize and streamline the testing
process within the Git project, improving code quality and
maintainability.
Next Steps:
I am eager to discuss these suggestions and collaborate with the Git
community to ensure the success of this project. I will continue to
engage with the community, seek guidance, and refine my proposal as
per your suggestions.
I look forward to the opportunity to contribute to the Git project
and help make it even more robust and reliable.
Best Regards,
Achu Luma
On Fri, Oct 20, 2023 at 2:15 PM Achu Luma <ach.lumap@gmail.com> wrote:
>
> Dear Git Community and Mentors,
>
> I hope this email finds you well. As a follow-up to my previous application, I'd like to provide additional details on the process of migrating existing unit tests from the t/helper/ directory to the new Git unit test framework.
> -- Identify Target Unit Tests: Start by identifying the specific unit tests in the t/helper/ directory that we want to port to the new Git unit test framework. Ensure that the tests are suitable for migration and that the benefits of doing so outweigh the effort(By avoiding integration tests). The following points have been developed with on going work on the unit-tests framework visible here: 1- https://lore.kernel.org/git/0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com/
> 2- https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc
>
> -- Create a New C Test File: For each unit test I plan to migrate, create a new C source file (.c) in the Git project's test suite directory(t/unit-tests). Name it appropriately to reflect the purpose of the test.
>
> -- Include Necessary Headers:In the new C test file, include the necessary Git unit test framework headers. Typically, this includes headers like "test-lib.h" and others relevant to the specific test.
> #include "test-lib.h"
>
> -- Convert Test Logic: Refactor the test logic from the original Shell script into the new C-based test format. Use the testing macros provided by the Git unit test framework, such as test_expect_success, test_expect_failure, etc., to define the tests.
> test_expect_success("simple progress display", "{
> // Test logic here...
> }");
>
> -- Add Test Descriptions: Provide clear and informative descriptions for each test using the testing macros. These descriptions will help in identifying the purpose of each test when the test suite is run.
>
> -- Define a Test Entry Point: Create a cmd_main function as the entry point for the C-based tests. Inside this function, include the test functions using the testing macros.
> int cmd_main(int argc, const char **argv) {
> // Test functions...
> return test_done();
> }
>
> -- Ensure TAP Format Output: Ensure that the C-based tests produce output in the Test Anything Protocol (TAP) format. This format includes the test name, status (ok or not ok), and any diagnostic information.
>
> -- Test Interaction: Ensure that the migrated tests interact correctly with the new Git unit test framework and any other tests that may be relevant. Consider dependencies and interactions with other parts of the Git project.
>
> -- Test Execution: Run the migrated tests to verify that they produce the expected results when executed as part of the Git project's test suite. Use the Git testing framework's test runners to execute the tests.
>
> -- Documentation Update: Update the Git project's documentation to reflect the changes made during the migration. Include a reference to the original unit tests in the t/helper/ directory and indicate that these tests have been ported to the new Git unit test framework.
>
> By following these points, I think I can successfully port existing unit tests from the t/helper/ directory to use the new Git unit test framework. This migration helps standardize and streamline the testing process within the Git project, improving code quality and maintainability.
>
> Next Steps:
>
> I am eager to discuss these suggestions and collaborate with the Git community to ensure the success of this project. I will continue to engage with the community, seek guidance, and refine my proposal as per your suggestions.
> I look forward to the opportunity to contribute to the Git project and help make it even more robust and reliable.
>
> Best Regards,
> Achu Luma
>
> On Mon, Oct 9, 2023 at 10:15 AM Luma <ach.lumap@gmail.com> wrote:
>>
>> Dear Git Community and Mentors,
>>
>> I hope this email finds you well. My name is Achu Luma, and I am
>> excited to submit my application for the Outreachy program with the
>> Git project.
>> I have been a passionate open-source enthusiast and a dedicated Git
>> user for two years, and I am thrilled at the opportunity to contribute
>> to the Git community.
>>
>> Introduction:
>> ----------------
>> I study Computer Science from the University of Bamenda. Over the past
>> 4 years, I have gained experience in software development and have
>> participated in various class projects.
>>
>> Why I am a Good Fit:
>> ----------------------
>> 1. Proficient with Git: I have a good understanding of Git's version
>> control system and have successfully used it in both personal and
>> educational projects.
>>
>> 2. Strong Programming Skills: My programming skills in python, C etc
>> and experience with git, shell etc make me well-prepared to contribute
>> to Git's codebase.
>>
>> 3. Open Source Involvement: I have actively contributed to git
>> open-source project, including
>> https://public-inbox.org/git/20231003174853.1732-1-ach.lumap@gmail.com/T/#t
>> , where I have submitted a patch that has been well-received.
>>
>> Project Idea - Moving Existing Tests to a Unit Testing Framework:
>> ------------------------------------------------------------------
>> I am excited about "Moving Existing Tests to a Unit Testing Framework".
>> The objective of this project is to enhance the efficiency and
>> maintainability of Git's testing infrastructure by porting existing
>> unit tests to a unit testing framework.
>>
>> **Project Plan**:
>> - Evaluate the existing tests in the `t/helper/` directory to identify
>> those suitable for migration to the unit testing framework.
>> - Develop a migration strategy and create detailed plans for adapting
>> these tests.
>> - Port the identified tests to the unit testing framework while
>> ensuring they maintain their functionality.
>> - Verify the correctness and reliability of the migrated tests through
>> thorough testing and validation.
>> - Collaborate with the Git community to gather feedback and make
>> necessary adjustments.
>>
>> **Timeline**:
>> - Community Bonding (Oct 2 - Nov 20): Familiarize myself with the Git
>> project and establish communication channels.
>> - Coding Phase (Dec 4 - Jan 15): Implement the migration of tests and
>> seek feedback from mentors and the community.
>> - Testing and Validation (Jan 15 - Feb 15): Rigorously test the
>> migrated tests and make improvements based on feedback.
>> - Documentation and Finalization (Feb 15 - March 1): Document the
>> migration process and finalize the project.
>>
>> **Contribution to Git Community**:
>> I have actively participated in Git's mailing-list discussions and
>> submitted a patch(
>> https://public-inbox.org/git/20231003174853.1732-1-ach.lumap@gmail.com/T/#t)
>> for review. I have received positive feedback on my contributions, and
>> it has been queued to be merged into official Git branches maintained
>> by Junio. Additionally, I have been involved in discussions related to
>> the git project.(https://public-inbox.org/git/CAFR+8DyN8vbuvdgZPkSVqS2=sqconwhx3QfcpJ0+Wi_oCA=s0w@mail.gmail.com/T/#t)
>>
>> **Proposal Drafts**:
>> I have shared drafts of this proposal on the Git mailing list
>> git@vger.kernel.org and will incorporate valuable feedback provided
>> by the community.
>>
>> **Next Steps**:
>> I am eager to discuss my proposal further and collaborate with the Git
>> community to ensure the success of this project. I will continue to
>> engage with the community, seek guidance, and refine my proposal as
>> per your suggestions.
>>
>> Thank you for considering my application. I look forward to the
>> opportunity to contribute to the Git project and help make it even
>> more robust and reliable.
>>
>> Best Regards,
>>
>> On Wed, Oct 4, 2023 at 12:36 AM Luma <ach.lumap@gmail.com> wrote:
>> >
>> > oh yes, "Move existing tests to a unit testing framework" was the
>> > only listed project for this current Outreachy cohort. So, I used it
>> > to express my intent.
>> > I appreciate the clarification on authorship identity for patches. I
>> > will update subsequent patches with a legal full name to conform to
>> > the community rules.
>> >
>> > Regards.
>> >
>> > On Tue, Oct 3, 2023 at 7:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>> > >
>> > > Luma <ach.lumap@gmail.com> writes:
>> > >
>> > > > Hi;
>> > > > My name is Luma, and I wanted to take a moment to introduce myself
>> > > > and share some
>> > > > insights on an essential aspect of avoiding pipes in git related
>> > > > commands in test scripts.
>> > > >
>> > > > I am an outreachy applicant for the December 2023 cohort and look
>> > > > forward to learning from you.
>> > >
>> > > I notice that the title of the message and the immediate topic you
>> > > discuss in the body of the message do not match. I presume that the
>> > > topic on the title is what you prefer to work on if the unit testing
>> > > framework is ready by the time Outreachy program starts, and the
>> > > mention about "do not clobber exit code of Git with pipes in the
>> > > tests" is your "dip the tip of a toe in water" microproject?
>> > >
>> > > Welcome to the Git development community.
>> > >
>> > > Do you have a single word name? If so please disregard the below,
>> > > but in case "Luma" is just a nickname (e.g. like I am introducing
>> > > myself to my Git friends "Hi, I am Gitster!") you use online, please
>> > > read on.
>> > >
>> > > For signing off your patches, we'd prefer to see your real name
>> > > used, as Signed-off-by: is meant to have legal significance. And
>> > > because we also expect the authorship identity to match the
>> > > name/e-mail of the sign-off, it would mean your patch submissions
>> > > are expected to look like:
>> > >
>> > > From: Luma <ach.lumap@gmail.com>
>> > > Subject: ... title of the patch goes here ...
>> > >
>> > > ... body of the proposed commit log message goes here...
>> > >
>> > > Signed-off-by: Luma <ach.lumap@gmail.com>
>> > >
>> > > but "Luma" replaced with your full real name.
>> > >
>> > > Thanks.
^ permalink raw reply
* Re: [PATCH v3 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-20 11:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqqttqmtcc2.fsf@gitster.g>
On Fri, Oct 20, 2023 at 1:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Do we really need to allocate a new bit in the object flags, which
> > is already a scarce resource?
>
> Clarification. I was *not* wondering if we can steal and (re|ab)use
> a bit that is used for other purposes, in order to avoid allocating
> a new bit.
>
> Rather, I was wondering if we need to use object flags to mark these
> objects, or can do what we want to do without using any object flags
> at all. For the purpose of reporting "missing" objects, wouldn't it
> be sufficient to walk the object graph and report our findings as we
> go? To avoid reporting the same object twice, as we reasonably can
> expect that the missing objects are minority (compared to the total
> number of objects), perhaps the codepath that makes such a report
> can use a hashmap of object_ids or something, for example.
>
This is kind of hard to do since the revision walking happens in revision.c
and list-objects.c but the missing commits are needed in builtin/rev-list.c.
So even if we build a list of missing commits in list-objects.c, there is no
nice way to send this back to rev-list.c.
The only way we communicate between these layers is through callbacks,
i.e. the show_commit() function in rev-list.c is called for every commit that
the revision walk traverses over.
I'm not entirely sure what the best path to take here to be honest. I will look
and see if there are any bits where overlapping doesn't cause any issues
in the meantime.
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Oswald Buddenhagen @ 2023-10-20 10:45 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <20231020101310.GB2673716@coredump.intra.peff.net>
On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:
>But one thing that gives me pause is that the neither before or after
>this patch do we handle continuation lines like:
>
> Subject: this is the beginning
> and this is more subject
>
>And it would probably be a lot easier to add when storing the headers in
>a hash (it's not impossible to do it the other way, but you basically
>have to delay processing each line with a small state machine).
>
that seems like a rather significant point, doesn't it?
>So another option is to just fix the individual bugs separately.
>
... so that seems preferable to me, given that the necessary fixes seem
rather trivial.
> I guess "readable" is up for debate here, but I find the inline handling
> a lot easier to follow
>
any particular reason for that?
> (and it's half as many lines; most of the diffstat is the new tests).
>- if ($parsed_email{'From'}) {
>- $sender = delete($parsed_email{'From'});
>- }
this verbosity could be cut down somewhat using just
$sender = delete($parsed_email{'From'});
and if the value can be pre-set and needs to be preserved,
$sender = delete($parsed_email{'From'}) // $sender;
but this seems kind of counter-productive legibility-wise.
regards
^ permalink raw reply
* Re: [PATCH 5/8] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
From: Jeff King @ 2023-10-20 10:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <xmqqcyxhxk0h.fsf@gitster.g>
On Sat, Oct 14, 2023 at 09:10:22AM -0700, Junio C Hamano wrote:
> > @@ -461,8 +449,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,
> > }
> >
> > if (s->commit_graph_read_changed_paths) {
> > + if (pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,
> > + &graph->chunk_bloom_indexes,
> > + st_mult(graph->num_commits, 4)) == -1)
> > + warning(_("commit-graph changed-path index chunk is too small (%d)"), graph->num_commits * 4);
> > read_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,
> > graph_read_bloom_data, graph);
> > }
>
> Overall the series looked sane, but the need for each caller to
> supply error messages, when the helper perfectly well knows how many
> bytes the caller expected and how many bytes there are avaiable, was
> a bit disturbing, as the level of detail given per each caller will
> inevitably become uneven. Even now, some give an error() while
> others give a warning(), even though I suspect all of them should be
> data errors.
>
> I wonder if it makes sense to stuff the message template in the
> pair_chunk_data structure and do
>
> static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> size_t chunk_size,
> void *data)
> {
> struct pair_chunk_data *pcd = data;
> if (pcd->expected_size != chunk_size)
> return error(_(pcd->message), pcd->expected_size, chunk_size);
> *pcd->p = chunk_start;
> return 0;
> }
One issue with the series as-is is that the "chunk is too small"
messages can be misleading when the chunk is in fact missing. We do say
"missing or corrupt" in the die message (at least for midx; I did not
update the similar ones for commit-graph), but the explicit "too small"
for a missing chunk seems to me to cross the line.
The caller can distinguish the cases by the actual value returned from
pair_chunk_expect(), but doing so makes the code a lot longer and harder
to read.
Your suggestion above takes care of it naturally (in the same way that
the existing code does, which basically is emitting the same message in
each read_chunk callback).
-Peff
^ permalink raw reply
* Re: [PATCH 0/8] chunk-format: introduce `pair_chunk_expect()` convenience API
From: Jeff King @ 2023-10-20 10:23 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1697225110.git.me@ttaylorr.com>
On Fri, Oct 13, 2023 at 03:25:12PM -0400, Taylor Blau wrote:
> While reviewing this series, I noticed a couple of spots that would be
> helped by having a convenience function that stores the start of a
> chunk in a designated location *after* checking that the chunk has the
> expected size.
>
> I called this function `pair_chunk_expect()` and touched up seven spots
> that I think could benefit from having a convenience function like this.
>
> This applies directly on top of 'jk/chunk-bounds'. Thanks in advance for
> your review!
I'm still a little skeptical of this approach, just because I think it
it discourages adding further checks. And in particular, I was planning
to add monotonicity checks to the midx OIDF chunk based on the
discussion in the earlier thread. And likewise, I think I probably
should have put the commit-graph checks into its OIDF reader, rather
than saving them for verify_commit_graph_lite(). That would match the
way we check the validity of the other chunks.
I haven't actually started writing those patches, though, so I'm not
sure of the full details yet.
-Peff
^ permalink raw reply
* [PATCH 3/3] send-email: handle to/cc/bcc from --compose message
From: Jeff King @ 2023-10-20 10:15 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231020100343.GA2194322@coredump.intra.peff.net>
If the user writes a message via --compose, send-email will pick up
varius headers like "From", "Subject", etc and use them for other
patches as if they were specified on the command-line. But we don't
handle "To", "Cc", or "Bcc" this way; we just tell the user "those
aren't interpeted yet" and ignore them.
But it seems like an obvious thing to want, especially as the same
feature exists when the cover letter is generated separately by
format-patch. There it is gated behind the --to-cover option, but I
don't think we'd need the same control here; since we generate the
--compose template ourselves based on the existing input, if the user
leaves the lines unchanged then the behavior remains the same.
So let's fill in the implementation; like those other headers we already
handle, we just need to assign to the initial_* variables. The only
difference in this case is that they are arrays, so we'll feed them
through parse_address_line() to split them (just like we would when
reading a single string via prompting).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-send-email.txt | 11 ++++++-----
git-send-email.perl | 16 ++++++++++++++--
t/t9001-send-email.sh | 16 +++++++++++-----
3 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 021276329c..f4d7166275 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -68,11 +68,12 @@ This option may be specified multiple times.
Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
to edit an introductory message for the patch series.
+
-When `--compose` is used, git send-email will use the From, Subject,
-Reply-To, and In-Reply-To headers specified in the message. If the body
-of the message (what you type after the headers and a blank line) only
-contains blank (or Git: prefixed) lines, the summary won't be sent, but
-the headers mentioned above will be used unless they are removed.
+When `--compose` is used, git send-email will use the From, To, Cc, Bcc,
+Subject, Reply-To, and In-Reply-To headers specified in the message. If
+the body of the message (what you type after the headers and a blank
+line) only contains blank (or Git: prefixed) lines, the summary won't be
+sent, but the headers mentioned above will be used unless they are
+removed.
+
Missing From or In-Reply-To headers will be prompted for.
+
diff --git a/git-send-email.perl b/git-send-email.perl
index bbda2a931b..9e21b0b3f4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -861,6 +861,9 @@ sub get_patch_subject {
my $tpl_subject = $initial_subject || '';
my $tpl_in_reply_to = $initial_in_reply_to || '';
my $tpl_reply_to = $reply_to || '';
+ my $tpl_to = join(',', @initial_to);
+ my $tpl_cc = join(',', @initial_cc);
+ my $tpl_bcc = join(', ', @initial_bcc);
print $c <<EOT1, Git::prefix_lines("GIT: ", __(<<EOT2)), <<EOT3;
From $tpl_sender # This line is ignored.
@@ -872,6 +875,9 @@ sub get_patch_subject {
Clear the body content if you don't wish to send a summary.
EOT2
From: $tpl_sender
+To: $tpl_to
+Cc: $tpl_cc
+Bcc: $tpl_bcc
Reply-To: $tpl_reply_to
Subject: $tpl_subject
In-Reply-To: $tpl_in_reply_to
@@ -928,8 +934,14 @@ sub get_patch_subject {
} elsif (/^From:\s*(.+)\s*$/i) {
$sender = $1;
next;
- } elsif (/^(?:To|Cc|Bcc):/i) {
- print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n");
+ } elsif (/^To:\s*(.+)\s*$/i) {
+ @initial_to = parse_address_line($1);
+ next;
+ } elsif (/^Cc:\s*(.+)\s*$/i) {
+ @initial_cc = parse_address_line($1);
+ next;
+ } elsif (/^Bcc:/i) {
+ @initial_bcc = parse_address_line($1);
next;
}
print $c2 $_;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 9644ff5793..2e8e8137fb 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2522,7 +2522,7 @@ test_expect_success $PREREQ '--compose handles lowercase headers' '
test_expect_success $PREREQ '--compose handles to headers' '
write_script fake-editor <<-\EOF &&
- sed "s/^$/To: edited-to@example.com\n/" <"$1" >"$1.tmp" &&
+ sed "s/^To: .*/&, edited-to@example.com/" <"$1" >"$1.tmp" &&
echo this is the body >>"$1.tmp" &&
mv "$1.tmp" "$1"
EOF
@@ -2534,10 +2534,16 @@ test_expect_success $PREREQ '--compose handles to headers' '
--to=nobody@example.com \
--smtp-server="$(pwd)/fake.sendmail" \
HEAD^ &&
- # Ideally the "to" header we specified would be used,
- # but the program explicitly warns that these are
- # ignored. For now, just make sure we did not abort.
- grep "To:" msgtxt1
+ # Check both that the cover letter used our modified "to" line,
+ # but also that it was picked up for the patch.
+ q_to_tab >expect <<-\EOF &&
+ To: nobody@example.com,
+ Qedited-to@example.com
+ EOF
+ grep -A1 "^To:" msgtxt1 >msgtxt1.to &&
+ test_cmp expect msgtxt1.to &&
+ grep -A1 "^To:" msgtxt2 >msgtxt2.to &&
+ test_cmp expect msgtxt2.to
'
test_done
--
2.42.0.980.g8b5f6199be
^ permalink raw reply related
* [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Jeff King @ 2023-10-20 10:13 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231020100343.GA2194322@coredump.intra.peff.net>
This reverts commit b6049542b97e7b135e0e82bf996084d461224d32.
Prior to that commit, we read the results of the user editing the
"--compose" message in a loop, picking out parts we cared about, and
streaming the result out to a ".final" file. That commit split the
reading/interpreting into two phases; we'd now read into a hash, and
then pick things out of the hash.
The goal was making the code more readable. And in some ways it did,
because the ugly regexes are confined to the reading phase. But it also
introduced several bugs, because now the two phases need to match each
other. In particular:
- we pick out headers like "Subject: foo" with a case-insensitive
regex, and then use the user-provided header name as the key in a
case-sensitive hash. So if the user wrote "subject: foo", we'd no
longer recognize it as a subject.
- the namespace for the hash keys conflates header names with meta
information like "body". If you put "body: foo" in your message, it
would be misinterpreted as the actual message body (nobody is likely
to do that in practice, but it seems like an unnecessary danger).
- the handling for to/cc/bcc is totally broken. The behavior before
that commit is to recognize and skip those headers, with a note to
the user that they are not yet handled. Not great, but OK. But
after the patch, the reading side now splits the addresses into a
perl array-ref. But the interpreting side doesn't handle this at
all, and blindly prints the stringified array-ref value. This leads
to garbage like:
(mbox) Adding to: ARRAY (0x555b4345c428) from line 'To: ARRAY(0x555b4345c428)'
error: unable to extract a valid address from: ARRAY (0x555b4345c428)
What to do with this address? ([q]uit|[d]rop|[e]dit):
Probably not a huge deal, since nobody should even try to use those
headers in the first place (since they were not implemented). But
the new behavior is worse, and indicative of the sorts of problems
that come from having the two layers.
The revert had a few conflicts, due to later work in this area from
15dc3b9161 (send-email: rename variable for clarity, 2018-03-04) and
d11c943c78 (send-email: support separate Reply-To address, 2018-03-04).
I've ported the changes from those commits over as part of the conflict
resolution.
The new tests show the bugs. Note the use of GIT_SEND_EMAIL_NOTTY in the
second one. Without it, the test is happy to reach outside the test
harness to the developer's actual terminal (when run with the buggy
state before this patch).
Signed-off-by: Jeff King <peff@peff.net>
---
I guess "readable" is up for debate here, but I find the inline handling
a lot easier to follow (and it's half as many lines; most of the
diffstat is the new tests).
But one thing that gives me pause is that the neither before or after
this patch do we handle continuation lines like:
Subject: this is the beginning
and this is more subject
And it would probably be a lot easier to add when storing the headers in
a hash (it's not impossible to do it the other way, but you basically
have to delay processing each line with a small state machine).
So another option is to just fix the individual bugs separately.
git-send-email.perl | 120 ++++++++++++++----------------------------
t/t9001-send-email.sh | 35 ++++++++++++
2 files changed, 75 insertions(+), 80 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..bbda2a931b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -888,73 +888,59 @@ sub get_patch_subject {
do_edit($compose_filename);
}
+ open my $c2, ">", $compose_filename . ".final"
+ or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!);
+
open $c, "<", $compose_filename
or die sprintf(__("Failed to open %s: %s"), $compose_filename, $!);
+ my $need_8bit_cte = file_has_nonascii($compose_filename);
+ my $in_body = 0;
+ my $summary_empty = 1;
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-
- my %parsed_email;
- while (my $line = <$c>) {
- next if $line =~ m/^GIT:/;
- parse_header_line($line, \%parsed_email);
- if ($line =~ /^$/) {
- $parsed_email{'body'} = filter_body($c);
+ while(<$c>) {
+ next if m/^GIT:/;
+ if ($in_body) {
+ $summary_empty = 0 unless (/^\n$/);
+ } elsif (/^\n$/) {
+ $in_body = 1;
+ if ($need_8bit_cte) {
+ print $c2 "MIME-Version: 1.0\n",
+ "Content-Type: text/plain; ",
+ "charset=$compose_encoding\n",
+ "Content-Transfer-Encoding: 8bit\n";
+ }
+ } elsif (/^MIME-Version:/i) {
+ $need_8bit_cte = 0;
+ } elsif (/^Subject:\s*(.+)\s*$/i) {
+ $initial_subject = $1;
+ my $subject = $initial_subject;
+ $_ = "Subject: " .
+ quote_subject($subject, $compose_encoding) .
+ "\n";
+ } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
+ $initial_in_reply_to = $1;
+ next;
+ } elsif (/^Reply-To:\s*(.+)\s*$/i) {
+ $reply_to = $1;
+ } elsif (/^From:\s*(.+)\s*$/i) {
+ $sender = $1;
+ next;
+ } elsif (/^(?:To|Cc|Bcc):/i) {
+ print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n");
+ next;
}
+ print $c2 $_;
}
close $c;
+ close $c2;
- open my $c2, ">", $compose_filename . ".final"
- or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!);
-
-
- if ($parsed_email{'From'}) {
- $sender = delete($parsed_email{'From'});
- }
- if ($parsed_email{'In-Reply-To'}) {
- $initial_in_reply_to = delete($parsed_email{'In-Reply-To'});
- }
- if ($parsed_email{'Reply-To'}) {
- $reply_to = delete($parsed_email{'Reply-To'});
- }
- if ($parsed_email{'Subject'}) {
- $initial_subject = delete($parsed_email{'Subject'});
- print $c2 "Subject: " .
- quote_subject($initial_subject, $compose_encoding) .
- "\n";
- }
-
- if ($parsed_email{'MIME-Version'}) {
- print $c2 "MIME-Version: $parsed_email{'MIME-Version'}\n",
- "Content-Type: $parsed_email{'Content-Type'};\n",
- "Content-Transfer-Encoding: $parsed_email{'Content-Transfer-Encoding'}\n";
- delete($parsed_email{'MIME-Version'});
- delete($parsed_email{'Content-Type'});
- delete($parsed_email{'Content-Transfer-Encoding'});
- } elsif (file_has_nonascii($compose_filename)) {
- my $content_type = (delete($parsed_email{'Content-Type'}) or
- "text/plain; charset=$compose_encoding");
- print $c2 "MIME-Version: 1.0\n",
- "Content-Type: $content_type\n",
- "Content-Transfer-Encoding: 8bit\n";
- }
- # Preserve unknown headers
- foreach my $key (keys %parsed_email) {
- next if $key eq 'body';
- print $c2 "$key: $parsed_email{$key}";
- }
-
- if ($parsed_email{'body'}) {
- print $c2 "\n$parsed_email{'body'}\n";
- delete($parsed_email{'body'});
- } else {
+ if ($summary_empty) {
print __("Summary email is empty, skipping it\n");
$compose = -1;
}
-
- close $c2;
-
} elsif ($annotate) {
do_edit(@files);
}
@@ -1009,32 +995,6 @@ sub ask {
return;
}
-sub parse_header_line {
- my $lines = shift;
- my $parsed_line = shift;
- my $addr_pat = join "|", qw(To Cc Bcc);
-
- foreach (split(/\n/, $lines)) {
- if (/^($addr_pat):\s*(.+)$/i) {
- $parsed_line->{$1} = [ parse_address_line($2) ];
- } elsif (/^([^:]*):\s*(.+)\s*$/i) {
- $parsed_line->{$1} = $2;
- }
- }
-}
-
-sub filter_body {
- my $c = shift;
- my $body = "";
- while (my $body_line = <$c>) {
- if ($body_line !~ m/^GIT:/) {
- $body .= $body_line;
- }
- }
- return $body;
-}
-
-
my %broken_encoding;
sub file_declares_8bit_cte {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 263db3ad17..9644ff5793 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2505,4 +2505,39 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' '
HEAD^
'
+test_expect_success $PREREQ '--compose handles lowercase headers' '
+ write_script fake-editor <<-\EOF &&
+ sed "s/^From:.*/from: edited-from@example.com/i" "$1" >"$1.tmp" &&
+ mv "$1.tmp" "$1"
+ EOF
+ clean_fake_sendmail &&
+ git send-email \
+ --compose \
+ --from="Example <from@example.com>" \
+ --to=nobody@example.com \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ HEAD^ &&
+ grep "From: edited-from@example.com" msgtxt1
+'
+
+test_expect_success $PREREQ '--compose handles to headers' '
+ write_script fake-editor <<-\EOF &&
+ sed "s/^$/To: edited-to@example.com\n/" <"$1" >"$1.tmp" &&
+ echo this is the body >>"$1.tmp" &&
+ mv "$1.tmp" "$1"
+ EOF
+ clean_fake_sendmail &&
+ GIT_SEND_EMAIL_NOTTY=1 \
+ git send-email \
+ --compose \
+ --from="Example <from@example.com>" \
+ --to=nobody@example.com \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ HEAD^ &&
+ # Ideally the "to" header we specified would be used,
+ # but the program explicitly warns that these are
+ # ignored. For now, just make sure we did not abort.
+ grep "To:" msgtxt1
+'
+
test_done
--
2.42.0.980.g8b5f6199be
^ permalink raw reply related
* [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose
From: Jeff King @ 2023-10-20 10:09 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231020100343.GA2194322@coredump.intra.peff.net>
The documentation for git-send-email lists the headers handled specially
by --compose in a way that implies that this is the complete set of
headers that are special. But one more was added by d11c943c78
(send-email: support separate Reply-To address, 2018-03-04) and never
documented.
Let's add it, and reword the documentation slightly to avoid having to
specify the list of headers twice (as it is growing and will continue to
do so as we add new features).
If you read the code, you may notice that we also handle MIME-Version
specially, in that we'll avoid over-writing user-provided MIME headers.
I don't think this is worth mentioning, as it's what you'd expect to
happen (as opposed to the other headers, which are picked up to be used
in later emails). And certainly this feature existed when the
documentation was expanded in 01d3861217 (git-send-email.txt: describe
--compose better, 2009-03-16), and we chose not to mention it then.
Signed-off-by: Jeff King <peff@peff.net>
---
Just something I noticed since a later commit touches the same list.
Documentation/git-send-email.txt | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 492a82323d..021276329c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -68,11 +68,11 @@ This option may be specified multiple times.
Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
to edit an introductory message for the patch series.
+
-When `--compose` is used, git send-email will use the From, Subject, and
-In-Reply-To headers specified in the message. If the body of the message
-(what you type after the headers and a blank line) only contains blank
-(or Git: prefixed) lines, the summary won't be sent, but From, Subject,
-and In-Reply-To headers will be used unless they are removed.
+When `--compose` is used, git send-email will use the From, Subject,
+Reply-To, and In-Reply-To headers specified in the message. If the body
+of the message (what you type after the headers and a blank line) only
+contains blank (or Git: prefixed) lines, the summary won't be sent, but
+the headers mentioned above will be used unless they are removed.
+
Missing From or In-Reply-To headers will be prompted for.
+
--
2.42.0.980.g8b5f6199be
^ permalink raw reply related
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Uwe Kleine-König @ 2023-10-20 10:04 UTC (permalink / raw)
To: git; +Cc: Luben Tuikov, Michael Strawbridge, entwicklung
In-Reply-To: <20231013141437.ywrhw65xdapmev7d@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]
hello,
On Fri, Oct 13, 2023 at 04:14:37PM +0200, Uwe Kleine-König wrote:
> Hello,
>
> $ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
> Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
> error: unable to extract a valid address from:
>
> This happens for me with git 2.42.0 and also on master (59167d7d09fd, "The seventeenth batch").
>
> Bisection points at
>
> a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
>
> I didn't try to understand that change and fix the problem.
Another (similar?) problem with non-ascii-chars:
$ git send-email --to 'Will Deacon <will@kernel.org>' --to 'Krzysztof Wilczyński <kw@linux.com>' --to 'Lorenzo Pieralisi <lpieralisi@kernel.org>' --cc 'Rob Herring <robh@kernel.org>' --to 'Bjorn Helgaas <bhelgaas@google.com>' --cc 'linux-pci@vger.kernel.org' --cc kernel@pengutronix.de -1 --base=@~
Use of uninitialized value $address in sprintf at /home/uwe/gsrc/git/git-send-email line 1162.
error: unable to extract a valid address from:
Bisection points to the same commit, when dropping ń in Krzysztof's
name, it works fine.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH 0/3] some send-email --compose fixes
From: Jeff King @ 2023-10-20 10:03 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <20231020071402.GC1642714@coredump.intra.peff.net>
[culling the rather large cc, as we moving off the original topic]
On Fri, Oct 20, 2023 at 03:14:03AM -0400, Jeff King wrote:
> and there's your perl array ref (from the square brackets, which are
> necessary because we're sticking it in a hash value). But even before
> your patch, this seems to end up as garbage. The code which reads
> $parsed_line does not dereference the array.
>
> The patch to fix it is only a few lines (well, more than that with some
> light editorializing in the comments):
So here's the fix in a cleaned up form, guided by my own comments from
earlier. ;) I think this is actually all orthogonal to the patch you are
working on, so yours could either go on top or just be applied
separately.
[1/3]: doc/send-email: mention handling of "reply-to" with --compose
[2/3]: Revert "send-email: extract email-parsing code into a subroutine"
[3/3]: send-email: handle to/cc/bcc from --compose message
Documentation/git-send-email.txt | 10 +--
git-send-email.perl | 132 ++++++++++++-------------------
t/t9001-send-email.sh | 41 ++++++++++
3 files changed, 98 insertions(+), 85 deletions(-)
-Peff
^ permalink raw reply
* Re: [PATCH] commit: detect commits that exist in commit-graph but not in the ODB
From: Jeff King @ 2023-10-20 10:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git, Karthik Nayak, Taylor Blau
In-Reply-To: <xmqqzg0ey1kn.fsf@gitster.g>
On Thu, Oct 19, 2023 at 10:16:56AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > There's another way to handle this, which is to conditionally enable the
> > object existence check. This would be less of a performance hit compared
> > to disabling commit graphs altogether with `--missing`, but still ensure
> > that repository corruption was detected. Second, it would not regress
> > performance for all preexisting users of `repo_parse_commit_gently()`.
>
> The above was what I meant to suggest when you demonstrated that the
> code with additional check is still much more performant than
> running without the commit-graph optimization, yet has observable
> impact on performance for normal codepaths that do not need the
> extra carefulness.
>
> But I wasn't sure if it is easy to plumb the "do we want to double
> check? in other words, are we running something like --missing that
> care the correctness a bit more than usual cases?" bit down from the
> caller, because this check is so deep in the callchain.
I wonder if we would want a "be extra careful" flag that is read from
the environment? That is largely how GIT_REF_PARANOIA works, and then
particular operations set it (though actually it is the default these
days, so they no longer do so explicitly).
I guess that is really like a global variable but even more gross. ;)
But it is nice that it can cross process boundaries, because "how
careful do we want to be" may be in the eye of the caller, especially
for plumbing commands. E.g., even without --missing, you may want
"rev-list" to be extra careful about checking the existence of commits.
The caller in check_connected() could arguably turn that on by default
(the way we used to turn on GIT_REF_PARANOIA for pruning repacks before
it became the default).
-Peff
^ permalink raw reply
* Re: [PATCH v2] builtin/branch.c: adjust error messages to coding guidelines
From: Christian Couder @ 2023-10-20 9:41 UTC (permalink / raw)
To: Isoken Ibizugbe; +Cc: git, gitster
In-Reply-To: <CAJHH8bFXVnFgjoCD+JU2uw77JAWUiKU+G=ub9Xg7xYJ_MwXPQQ@mail.gmail.com>
Hi Isoken,
On Fri, Oct 20, 2023 at 6:28 AM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
>
> On Thu, Oct 19, 2023 at 9:41 AM Isoken June Ibizugbe
> <isokenjune@gmail.com> wrote:
> >
> > As per the CodingGuidelines document, it is recommended that a single-line
> > message provided to error messages such as die(), error() and warning(),
> > should start with a lowercase letter and should not end with a period.
> > Also this patch fixes the tests broken by the changes.
> >
> > Signed-off-by: Isoken June Ibizugbe <isokenjune@gmail.com>
> > ---
> > builtin/branch.c | 66 +++++++++++++++++++--------------------
> > t/t2407-worktree-heads.sh | 2 +-
> > t/t3200-branch.sh | 16 +++++-----
> > t/t3202-show-branch.sh | 10 +++---
> > 4 files changed, 47 insertions(+), 47 deletions(-)
> Hello Junio, I would appreciate your comment on this.
Please don't expect Junio to give direct feedback on all the patches
sent to the mailing list. This patch has already been reviewed by
Rubén. If Junio doesn't review it and if this patch is not mentioned
in the next "What's cooking in git.git" email from Junio, it likely
means that you should send a version 3 addressing Rubén's feedback
about the commit message.
Thanks,
Christian.
^ permalink raw reply
* [PATCH v3 1/3] rebase: simplify code related to imply_merge()
From: Oswald Buddenhagen @ 2023-10-20 9:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood
In-Reply-To: <20231020093654.922890-1-oswald.buddenhagen@gmx.de>
The code's evolution left in some bits surrounding enum rebase_type that
don't really make sense any more. In particular, it makes no sense to
invoke imply_merge() if the type is already known not to be
REBASE_APPLY, and it makes no sense to assign the type after calling
imply_merge().
enum rebase_type had more values until commit a74b35081c ("rebase: drop
support for `--preserve-merges`") and commit 10cdb9f38a ("rebase: rename
the two primary rebase backends"). The latter commit also renamed
imply_interactive() to imply_merge().
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2:
- more verbose commit message
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
builtin/rebase.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..44cc1eed12 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -386,7 +386,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
opts->keep_empty = !unset;
- opts->type = REBASE_MERGE;
return 0;
}
@@ -1505,9 +1504,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
}
- if (options.type == REBASE_MERGE)
- imply_merge(&options, "--merge");
-
if (options.root && !options.onto_name)
imply_merge(&options, "--root without --onto");
@@ -1552,7 +1548,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
- imply_merge(&options, "--merge");
+ options.type = REBASE_MERGE;
else if (!strcmp(options.default_backend, "apply"))
options.type = REBASE_APPLY;
else
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related
* [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down
From: Oswald Buddenhagen @ 2023-10-20 9:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood
In-Reply-To: <20231020093654.922890-1-oswald.buddenhagen@gmx.de>
This moves it right next to parse_opt_empty(), which is a much more
logical place. As a side effect, this removes the need for a forward
declaration of imply_merge().
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
builtin/rebase.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a093bb125..13ca5a644b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -376,19 +376,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)
return ret;
}
-static void imply_merge(struct rebase_options *opts, const char *option);
-static int parse_opt_keep_empty(const struct option *opt, const char *arg,
- int unset)
-{
- struct rebase_options *opts = opt->value;
-
- BUG_ON_OPT_ARG(arg);
-
- imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
- opts->keep_empty = !unset;
- return 0;
-}
-
static int is_merge(struct rebase_options *opts)
{
return opts->type == REBASE_MERGE;
@@ -982,6 +969,18 @@ static enum empty_type parse_empty_value(const char *value)
die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
}
+static int parse_opt_keep_empty(const struct option *opt, const char *arg,
+ int unset)
+{
+ struct rebase_options *opts = opt->value;
+
+ BUG_ON_OPT_ARG(arg);
+
+ imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
+ opts->keep_empty = !unset;
+ return 0;
+}
+
static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
{
struct rebase_options *options = opt->value;
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related
* [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well
From: Oswald Buddenhagen @ 2023-10-20 9:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood
In-Reply-To: <20231020093654.922890-1-oswald.buddenhagen@gmx.de>
At least after the successive trimming of enum rebase_type mentioned in
the previous commit, this code did exactly what imply_merge() does, so
just call it instead.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
builtin/rebase.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 44cc1eed12..4a093bb125 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.strategy) {
options.strategy = xstrdup(options.strategy);
- switch (options.type) {
- case REBASE_APPLY:
- die(_("--strategy requires --merge or --interactive"));
- case REBASE_MERGE:
- /* compatible */
- break;
- case REBASE_UNSPECIFIED:
- options.type = REBASE_MERGE;
- break;
- default:
- BUG("unhandled rebase type (%d)", options.type);
- }
+ imply_merge(&options, "--strategy");
}
if (options.root && !options.onto_name)
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related
* [PATCH v3 0/3] rebase refactoring
From: Oswald Buddenhagen @ 2023-10-20 9:36 UTC (permalink / raw)
To: git
In-Reply-To: <20230809171531.2564844-1-oswald.buddenhagen@gmx.de>
broken out of the bigger series, as the aggregation just unnecessarily holds it
up.
v3: removed "stray" footer. so more of a RESEND than an actual new version.
Oswald Buddenhagen (3):
rebase: simplify code related to imply_merge()
rebase: handle --strategy via imply_merge() as well
rebase: move parse_opt_keep_empty() down
builtin/rebase.c | 44 ++++++++++++++------------------------------
1 file changed, 14 insertions(+), 30 deletions(-)
--
2.42.0.419.g70bf8a5751
^ permalink raw reply
* [RESEND] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-20 9:27 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Phillip Wood, Christian Couder, Charvi Mendiratta,
Marc Branchaud
Create a clear top-down structure which makes it hopefully unambiguous
what happens when.
Also mention the timestamp along with the author - this is primarily
meant to include the keywords somebody might be searching for, like I
did a year ago.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
Cc: Christian Couder <christian.couder@gmail.com>
Cc: Charvi Mendiratta <charvi077@gmail.com>
Cc: Marc Branchaud <marcnarc@xiplink.com>
---
Documentation/git-rebase.txt | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..857e025361 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -890,20 +890,21 @@ command "pick" with the command "reword".
To drop a commit, replace the command "pick" with "drop", or just
delete the matching line.
-If you want to fold two or more commits into one, replace the command
-"pick" for the second and subsequent commits with "squash" or "fixup".
-If the commits had different authors, the folded commit will be
-attributed to the author of the first commit. The suggested commit
-message for the folded commit is the concatenation of the first
-commit's message with those identified by "squash" commands, omitting the
-messages of commits identified by "fixup" commands, unless "fixup -c"
-is used. In that case the suggested commit message is only the message
-of the "fixup -c" commit, and an editor is opened allowing you to edit
-the message. The contents (patch) of the "fixup -c" commit are still
-incorporated into the folded commit. If there is more than one "fixup -c"
-commit, the message from the final one is used. You can also use
-"fixup -C" to get the same behavior as "fixup -c" except without opening
-an editor.
+If you want to fold two or more commits into one (that is, to combine
+their contents/patches), replace the command "pick" for the second and
+subsequent commits with "squash" or "fixup".
+The commit message for the folded commit is the concatenation of the
+first commit's message with those identified by "squash" commands,
+omitting the messages of commits identified by "fixup" commands, unless
+"fixup -c" is used. In the latter case, the message is obtained only
+from the "fixup -c" commit (having more than one "fixup -c" commit
+makes no sense, and only the message from the last one is used).
+If the resulting commit message is a concatenation of multiple messages,
+an editor is opened allowing you to edit it. This is also the case for a
+message obtained via "fixup -c", while using "fixup -C" instead skips
+the editor; this is analogous to the behavior of `git commit`.
+The first commit which contributes to the suggested commit message also
+determines the author, along with the date/timestamp.
`git rebase` will stop when "pick" has been replaced with "edit" or
when a command fails due to merge errors. When you are done editing
--
2.42.0.419.g70bf8a5751
^ 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