* Re: [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Phillip Wood @ 2023-10-25 14:44 UTC (permalink / raw)
To: Han-Wen Nienhuys, Junio C Hamano; +Cc: Patrick Steinhardt, git, Eric Sunshine
In-Reply-To: <CAFQ2z_PqNsz+zycSxz=q2cUVOpJS-AEjwHxEM-fiafxd3dxc9g@mail.gmail.com>
On 25/10/2023 15:26, Han-Wen Nienhuys wrote:
> On Tue, Oct 24, 2023 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>>> this patch series introduces a new `--exists` mode to git-show-ref(1) to
>>> explicitly check for the existence of a reference, only.
>>
>> I agree that show-ref would be the best place for this feature (not
>> rev-parse, which is already a kitchen sink). After all, the command
>> was designed for validating refs in 358ddb62 (Add "git show-ref"
>> builtin command, 2006-09-15).
>>
>> Thanks. Hopefully I can take a look before I go offline.
>
> The series description doesn't say why users would care about this.
>
> If this is just to ease testing, I suggest adding functionality to a
> suitable test helper. Anything you add to git-show-ref is a publicly
> visible API that needs documentation and comes with a stability
> guarantee that is more expensive to maintain than test helper
> functionality.
Does the new functionality provide a way for scripts to see if a branch
is unborn (i.e. has not commits yet)? I don't think we have a way to
distinguish between a ref that points to a missing object and an unborn
branch at the moment.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Han-Wen Nienhuys @ 2023-10-25 14:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Steinhardt, git, Eric Sunshine
In-Reply-To: <xmqqttqf3k5a.fsf@gitster.g>
On Tue, Oct 24, 2023 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Patrick Steinhardt <ps@pks.im> writes:
>
> > this patch series introduces a new `--exists` mode to git-show-ref(1) to
> > explicitly check for the existence of a reference, only.
>
> I agree that show-ref would be the best place for this feature (not
> rev-parse, which is already a kitchen sink). After all, the command
> was designed for validating refs in 358ddb62 (Add "git show-ref"
> builtin command, 2006-09-15).
>
> Thanks. Hopefully I can take a look before I go offline.
The series description doesn't say why users would care about this.
If this is just to ease testing, I suggest adding functionality to a
suitable test helper. Anything you add to git-show-ref is a publicly
visible API that needs documentation and comes with a stability
guarantee that is more expensive to maintain than test helper
functionality.
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Liana Sebastian
^ permalink raw reply
* Re: [RFC][Outreachy] Seeking Git Community Feedback on My Application
From: Isoken Ibizugbe @ 2023-10-25 12:45 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD22EpdBU8HJqFM+=75EBABOTf5a0q+KsbzLK+XTEGSkPw@mail.gmail.com>
On Mon, Oct 23, 2023 at 3:24 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Thu, Oct 19, 2023 at 11:26 AM Isoken Ibizugbe <isokenjune@gmail.com> wrote:
> >
> > Dear Git Community and Mentors,
> >
> > I hope you're doing well. I'm excited to share my application draft
> > for the Outreachy program with the Git project. Your feedback is
> > invaluable, and I'm eager to align the project with the community's
> > needs. Please review the attached draft and share your insights.
>
> Thanks for your project application!
>
> [...]
>
> > Why am I interested in working with the Git chosen project?
> >
> > Git has been a cornerstone for software development, enabling
> > developers worldwide to collaborate, innovate, and create exceptional
> > software. I would say without Git, my journey to pursuing my software
> > engineering career would be impossible, as I use it almost every day.
> > Yet, in this constantly evolving landscape, there is always room for
> > improvement, even in a well-established project. The Git project
> > currently relies on end-to-end tests, and this is where I see an
> > opportunity to make a profound impact. Being able to test libraries in
> > isolation via unit tests or mocks speeds up determining the root cause
> > of bugs. I am deeply passionate about contributing to this project and
> > firmly believe in the power of open-source software and the collective
> > intelligence of the community. A successful completion of this project
> > will significantly improve Git's testing capabilities and bring the
> > benefits of fewer errors, faster work and better testing for all
> > parts.
>
> Ok.
>
> [...]
>
> > Contributions to Git
> >
> > I have actively participated in Git's mailing list discussions and
> > contributed to a micro-project;
> >
> > - builtin/branch.c: Adjust error messages such as die(), error(), and
> > warning() messages used in branch, to conform to coding guidelines
> > (https://lore.kernel.org/git/20231019084052.567922-1-isokenjune@gmail.com/)
> > - Implemented changes to fix broken tests based on reviews from the
> > community (https://lore.kernel.org/git/20231019084052.567922-1-isokenjune@gmail.com/)
> > - In review.
>
> Nice!
>
> > Project Goals:
> >
> > - Improve Testing Efficiency: Transitioning from end-to-end tests to
> > unit tests will enable more efficient testing of error conditions.
> > - Codebase Stability: Unit tests enhance code stability and facilitate
> > easier debugging through isolation.
> > - Simplify Testing: Writing unit tests in pure C simplifies test
> > setup, data passing, and reduces testing runtime by eliminating
> > separate processes for each test.
>
> Ok.
>
> > Project Milestones:
> >
> > - Add useful tests of library-like code
> > - Integrate with stdlib work
>
> Not sure what you call "stdlib" here.
>
> > - Run alongside regular make test target
> >
> > Project Timeline:
> >
> > 1. Oct 2 - Nov 20: Community Bonding
> >
> > - Understanding the structure of Git
> > - Getting familiar with the code
>
> I think some of this time is also spent on working on a microproject,
> writing an application and perhaps doing other things that regular Git
> developers do.
>
> > 2. Dec 4 - Jan 15: Add useful tests of library-like code
> >
> > - Identify and document the current state of the tests in the Git
> > t/helper directory.
>
> It would be nice if you could already take a look at that and tell us
> about it in your application. There are different things in t/helper.
> Some are worth porting and others are not. You might not want (or have
> time to) to classify everything right now, but if you can identify a
> few of each kind, and use those, or just one of them, as an example,
> that would be great.
>
> > - Confirm the licensing and compatibility requirements for the chosen
> > unit testing framework.
>
> I think those who have been working on the unit test framework have
> already done this.
Thank you for the review. I have made changes to the project plan and
it emphasizes the critical tasks of identifying, selecting, and
porting tests, making it more concise and aligned with the project's
scope.
- Community Bonding (Oct 2 - Nov 20): Microproject contribution, Git
project application, get familiar with the Git codebase and testing
ecosystem.
-Identify and Select Tests: Identify and prioritize tests worth
porting, and document the selected tests. (I would classify tests that
are worth porting according to the following for now;
Relevance: Prioritize tests that are relevant to the current Git codebase.
Coverage: Focus on tests that cover core functionality or critical code paths.
Usage Frequency: Port tests that are frequently used or run in Git's
development process.
Isolation: Choose tests that can be easily ported and run independently.
- Write Unit Tests: Write unit tests for the identified test cases
using the Git custom test framework.
- Port Existing Tests: Port selected test cases from the t/helper
directory to the unit testing framework, by modifying them to work
within the custom TAP framework.
- Test Execution and Debugging: Execute the newly written unit tests
and the ported tests using the test framework.
- Seek Feedback: Share the progress with mentors and the Git
community, and address any concerns or suggestions provided by the
community.
- Documentation and Reporting: Document the entire process of
migrating Git's tests to the unit testing framework, and prepare a
final project report summarizing the work done, challenges faced, and
lessons learned.
What is the custom TAP framework?
According to this patch
(https://lore.kernel.org/git/ca284c575ece0aee7149641d5fb1977ccd7e7873.1692229626.git.steadmon@google.com/)
by Phillip Wood, which contains an example implementation for writing
unit tests with TAP output. The custom TAP framework is a Test
Anything Protocol (TAP) framework that allows for clear reporting of
test results, aiding in debugging and troubleshooting.
The framework contains the following features:
- Test Structure: Unit tests are defined as functions containing
multiple checks. The tests are run using the TEST() macro. If any
checks within a test fail, the entire test is marked as failed.
- Output Format: The output of the test program follows the TAP
format. It includes a series of messages describing the test's status.
For passed tests, it reports "ok," and for failed tests, it reports
"not ok." Each test is numbered, e.g., "ok 1 - static initialization
works," to indicate success or failure.
- Check Functions: Several check functions are available, including
check() for boolean conditions, check_int(), check_uint(), and
check_char() for comparing values using various operators. check_str()
is used to compare strings.
- Skipping Tests: Tests can be skipped using test_skip() and can
include a reason for skipping, which is printed as part of the report.
- Diagnostic Messages: Tests can generate diagnostic messages using
test_msg() to provide additional context or explanations for test
failures.
- Planned Failing Tests: Tests that are known to fail can be marked
with TEST_TODO(). These tests will still run, and the failures will be
reported, but they will not cause the entire suite to fail.
- Building and Running: The unit tests can be built with "make
unit-tests" (with some additional Makefile changes), and they can be
executed manually or using a tool like prove.
Using the formerly given criteria, test-ctype.c is suitable for
porting because it tests character type checks used extensively in
Git. These tests cover various character types and their expected
behaviour, ensuring the correctness and reliability of Git's
operations, and test-ctype.c isolation makes it suitable for porting
without relying on multiple libraries.
Here is a sample of the implementation of how I would write the unit
test following the custom TAP framework taking t/helper/test-ctype.c
- Create and rename the new .c file;
I would rename it according to the convention done in the t/unit-test
directory, by starting the name with a “t-” prefix e.g t-ctype.c
- Document the tests and include the necessary headers:
/**
*Tests the behavior of ctype
*functions
*/
#include "test-lib.h"
#include "ctype.h"
- Define test functions:
#define DIGIT "0123456789"
static void t_digit_type(void)
{
int i;
const char *digits = DIGIT;
for (i = 0; digits[i]; i++)
{
check_int(isdigit(digits[i]), ==, 0);
}
- Include main function which will call the test functions using the TEST macro;
int main(void)
{
TEST(t_digit_type(), "Character is a digit");
return test_done();
}
- Run the tests:
‘make && make’ unit-tests can be used build and run the unit tests
Or run the test binaries directly:
./t/unit-tests/t-ctype.c
The Makefile will be modified to add the file;
UNIT_TEST_PROGRAMS += t-ctype
The test output will be in the TAP format and will indicate which
tests passed(ok) and which failed(not ok), along with diagnostic
messages in case of failures.
ok 1 - Character is a digit
1..1
>
> > - Develop unit tests for these library-like components.
>
> Not sure what are "these library-like components". An example would
> perhaps help.
>
> > - Execute the tests and ensure they cover various scenarios, including
> > error conditions.
> > - Run the tests and address any initial issues or bugs to ensure they
> > work as intended.
>
> Ok.
>
> > - Document the new tests and their coverage.
>
> What kind of documentation would that be?
>
> > - Seek feedback and support from mentors and the Git community
> >
> > 3. Jan 15 - Feb 15: Integrate with Stdlib Work
> >
> > - Collaborate with the team working on standard library integration.
>
> Not sure what "standard library". Actually, maybe you are talking
> about the goal of having a "standard library" implementation for Git
> which is described in this report from the Virtual Contributor's
> Summit:
>
> https://lore.kernel.org/git/ZRrfN2lbg14IOLiK@nand.local/
>
> It's true that the unit test framework would help with that goal. So
> yeah maybe you will have to collaborate with the team working on that
> goal. I am not sure at what step the work on this library will be when
> the internship will start though.
>
> > - Ensure that the tests for library-like code align with stdlib work.
> > - Verify that the tests effectively check the compatibility and
> > interaction of the code with standard libraries.
> > - Gather feedback and insights from the Git community on the
> > integrated tests, addressing any concerns or suggestions.
>
> Ok, but I think it would be more interesting to follow the steps with
> an example test.
>
> > 4. Feb 15 - March 1: Run Alongside Regular 'make test' Target and finalize
> >
> > - Configure the testing framework to run alongside the regular 'make
> > test' target.
>
> I think others will likely take care of that sooner.
>
> > - Ensure that the new tests are included in the standard testing suite.
> > - Execute 'make test' with the new tests and verify that they pass successfully.
> > - Document the integration process and how the new tests are included
> > in the standard testing procedure.
> > - Perform comprehensive testing of the entire unit testing framework.
> > - Ensure all migrated tests are working correctly within the new framework.
> > - Document the entire process of migrating Git's tests
> > - Prepare a final project report
>
> Ok, but here also following an example test would be more interesting.
>
> > Technical Requirements
> >
> > According to the documentation on the unit test project
> > (https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc),
> > the suggested best framework for the Git project is the "Custom TAP
> > framework" (Phillip Wood's TAP implementation), as it aligns with
> > Git's licensing requirements, is vendorable, and can be customized by
> > Git's developers as needed, but it may require some additional
> > development work for features like parallel execution and mock
> > support, but it offers a strong foundation for unit testing within the
> > Git project.
>
> Yeah, right. Thanks for summarizing that document!
>
> > Relevant Projects
> >
> > Simple shell - A project based on emulating a shell. It was a
> > collaborative project which we managed using Git.
> > (https://github.com/Junie06/simple_shell).
> > This project was written in C, which allowed me to apply my C language
> > knowledge, essential for Git projects.
> > I'm proficient in using Shell for scripting, redirections, and
> > permissions, as shown in my work
> > (https://github.com/Junie06/alx-system_engineering-devops).
> > Creating the simple shell project deepened my understanding of how
> > shells work, and I even attempted to replicate a shell environment.
> > Collaborating on the Simple Shell project reinforced my Git skills.
>
> Ok, nice!
>
> Best,
> Christian.
^ permalink raw reply
* [PATCH] merge: ignore whitespace changes when detecting renames
From: Stefan Muenzel via GitGitGadget @ 2023-10-25 12:14 UTC (permalink / raw)
To: git; +Cc: Stefan Muenzel, Stefan Muenzel
From: Stefan Muenzel <source@s.muenzel.net>
The options ignore-space-changes and ignore-all-space for
merge strategies based on "ort" are now propagated
to the computation of file renames, by normalizing spacing
changes when computing file hashes.
Signed-off-by: Stefan Muenzel <source@s.muenzel.net>
---
merge: ignore whitespace changes when detecting renames
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1600%2Fsmuenzel%2Frenames-ignore-whitespace-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1600/smuenzel/renames-ignore-whitespace-v1
Pull-Request: https://github.com/git/git/pull/1600
diff.c | 4 ++++
diffcore-delta.c | 14 ++++++++++++++
diffcore-rename.c | 9 +++++++++
diffcore.h | 2 ++
merge-recursive.c | 1 +
5 files changed, 30 insertions(+)
diff --git a/diff.c b/diff.c
index 2c602df10a3..916cc38f57c 100644
--- a/diff.c
+++ b/diff.c
@@ -4081,6 +4081,7 @@ int diff_populate_filespec(struct repository *r,
{
int size_only = options ? options->check_size_only : 0;
int check_binary = options ? options->check_binary : 0;
+ int ignore_whitespace = options ? options->ignore_whitespace : 0;
int err = 0;
int conv_flags = global_conv_flags_eol;
/*
@@ -4090,6 +4091,9 @@ int diff_populate_filespec(struct repository *r,
if (conv_flags & CONV_EOL_RNDTRP_DIE)
conv_flags = CONV_EOL_RNDTRP_WARN;
+ if (ignore_whitespace)
+ s->ignore_whitespace = 1;
+
if (!DIFF_FILE_VALID(s))
die("internal error: asking to populate invalid file.");
if (S_ISDIR(s->mode))
diff --git a/diffcore-delta.c b/diffcore-delta.c
index c30b56e983b..e2bb5f9133a 100644
--- a/diffcore-delta.c
+++ b/diffcore-delta.c
@@ -130,6 +130,7 @@ static struct spanhash_top *hash_chars(struct repository *r,
unsigned char *buf = one->data;
unsigned int sz = one->size;
int is_text = !diff_filespec_is_binary(r, one);
+ int ignore_whitespace = one->ignore_whitespace;
i = INITIAL_HASH_SIZE;
hash = xmalloc(st_add(sizeof(*hash),
@@ -149,6 +150,19 @@ static struct spanhash_top *hash_chars(struct repository *r,
if (is_text && c == '\r' && sz && *buf == '\n')
continue;
+ if (is_text && ignore_whitespace && isspace(c)) {
+ if (sz) {
+ char next = *buf;
+ if ( c == '\n' && next == '\n')
+ continue;
+ else if ( c != '\n' && isspace(next))
+ continue;
+ }
+ if ( c != '\n')
+ /* Normalize whitespace to space*/
+ c = ' ';
+ }
+
accum1 = (accum1 << 7) ^ (accum2 >> 25);
accum2 = (accum2 << 7) ^ (old_1 >> 25);
accum1 += c;
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 5a6e2bcac71..ee68442afc3 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -14,6 +14,7 @@
#include "string-list.h"
#include "strmap.h"
#include "trace2.h"
+#include "xdiff-interface.h"
/* Table of rename/copy destinations */
@@ -950,6 +951,10 @@ static int find_basename_matches(struct diff_options *options,
.info = info
};
+ dpf_options.ignore_whitespace =
+ DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE)
+ || DIFF_XDL_TST(options, IGNORE_WHITESPACE);
+
/*
* Create maps of basename -> fullname(s) for remaining sources and
* dests.
@@ -1402,6 +1407,10 @@ void diffcore_rename_extended(struct diff_options *options,
.repo = options->repo
};
+ dpf_options.ignore_whitespace =
+ DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE)
+ || DIFF_XDL_TST(options, IGNORE_WHITESPACE);
+
trace2_region_enter("diff", "setup", options->repo);
info.setup = 0;
assert(!dir_rename_count || strmap_empty(dir_rename_count));
diff --git a/diffcore.h b/diffcore.h
index 5ffe4ec788f..d6bee67e646 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -61,6 +61,7 @@ struct diff_filespec {
unsigned has_more_entries : 1; /* only appear in combined diff */
/* data should be considered "binary"; -1 means "don't know yet" */
signed int is_binary : 2;
+ unsigned ignore_whitespace : 1;
struct userdiff_driver *driver;
};
@@ -78,6 +79,7 @@ void diff_queued_diff_prefetch(void *repository);
struct diff_populate_filespec_options {
unsigned check_size_only : 1;
unsigned check_binary : 1;
+ unsigned ignore_whitespace : 1;
/*
* If an object is missing, diff_populate_filespec() will invoke this
diff --git a/merge-recursive.c b/merge-recursive.c
index e3beb0801b1..0e52c45158a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1894,6 +1894,7 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *opt,
repo_diff_setup(opt->repo, &opts);
opts.flags.recursive = 1;
opts.flags.rename_empty = 0;
+ opts.xdl_opts = opt->xdl_opts;
opts.detect_rename = merge_detect_rename(opt);
/*
* We do not have logic to handle the detection of copies. In
base-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 11/12] builtin/show-ref: add new mode to check for reference existence
From: Patrick Steinhardt @ 2023-10-25 11:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <CAPig+cRTOMie0rUf=Mhbo9e2EXf-_2kQyMeqpB9OCRB1MZZ1rw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6936 bytes --]
On Tue, Oct 24, 2023 at 05:01:55PM -0400, Eric Sunshine wrote:
> On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> > While we have multiple ways to show the value of a given reference, we
> > do not have any way to check whether a reference exists at all. While
> > commands like git-rev-parse(1) or git-show-ref(1) can be used to check
> > for reference existence in case the reference resolves to something
> > sane, neither of them can be used to check for existence in some other
> > scenarios where the reference does not resolve cleanly:
> >
> > - References which have an invalid name cannot be resolved.
> >
> > - References to nonexistent objects cannot be resolved.
> >
> > - Dangling symrefs can be resolved via git-symbolic-ref(1), but this
> > requires the caller to special case existence checks depending on
> > whteher or not a reference is symbolic or direct.
>
> s/whteher/whether/
>
> > Furthermore, git-rev-list(1) and other commands do not let the caller
> > distinguish easily between an actually missing reference and a generic
> > error.
> >
> > Taken together, this gseems like sufficient motivation to introduce a
>
> s/gseems/seems/
>
> > separate plumbing command to explicitly check for the existence of a
> > reference without trying to resolve its contents.
> >
> > This new command comes in the form of `git show-ref --exists`. This
> > new mode will exit successfully when the reference exists, with a
> > specific error code of 2 when it does not exist, or with 1 when there
> > has been a generic error.
> >
> > Note that the only way to properly implement this command is by using
> > the internal `refs_read_raw_ref()` function. While the public function
> > `refs_resolve_ref_unsafe()` can be made to behave in the same way by
> > passing various flags, it does not provide any way to obtain the errno
> > with which the reference backend failed when reading the reference. As
> > such, it becomes impossible for us to distinguish generic errors from
> > the explicit case where the reference wasn't found.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> > @@ -65,6 +70,12 @@ OPTIONS
> > +--exists::
> > +
> > + Check whether the given reference exists. Returns an error code of 0 if
>
> We probably want to call this "exit code" rather than "error code"
> since the latter is unnecessarily scary sounding for the success case
> (when the ref does exit).
I was trying to stick to the preexisting style of "error code" in this
manual page. But I think I agree with your argument that we also call it
an error code in the successful case, which is misleading.
> > + it does, 2 if it is missing, and 128 in case looking up the reference
> > + failed with an error other than the reference being missing.
>
> The commit message says it returns 1 for a generic error, but this
> inconsistently says it returns 128 for that case. The actual
> implementation returns 1.
Good catch, fixed.
> > diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> > @@ -214,6 +215,41 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
> > +static int cmd_show_ref__exists(const char **refs)
> > +{
> > + struct strbuf unused_referent = STRBUF_INIT;
> > + struct object_id unused_oid;
> > + unsigned int unused_type;
> > + int failure_errno = 0;
> > + const char *ref;
> > + int ret = 1;
> > +
> > + if (!refs || !*refs)
> > + die("--exists requires a reference");
> > + ref = *refs++;
> > + if (*refs)
> > + die("--exists requires exactly one reference");
> > +
> > + if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
> > + &unused_oid, &unused_referent, &unused_type,
> > + &failure_errno)) {
> > + if (failure_errno == ENOENT) {
> > + error(_("reference does not exist"));
>
> The documentation doesn't mention this printing any output, and indeed
> one would intuitively expect a boolean-like operation to not produce
> any printed output since its exit code indicates the result (except,
> of course, in the case of a real error).
I'm inclined to leave this as-is. While the exit code should be
sufficient, I think it's rather easy to wonder whether it actually did
anything at all and why it failed in more interactive use cases. Not
that I think these will necessarily exist.
I also don't think it's going to hurt to print this error. If it ever
does start to become a problem we might end up honoring the "--quiet"
flag to squelch this case.
> > + ret = 2;
> > + } else {
> > + error(_("failed to look up reference: %s"), strerror(failure_errno));
>
> Or use error_errno():
>
> errno = failure_errno;
> error_errno(_("failed to look up reference: %s"));
Ah, good suggestion.
> > + }
> > +
> > + goto out;
> > + }
> > +
> > + ret = 0;
> > +
> > +out:
> > + strbuf_release(&unused_referent);
> > + return ret;
> > +}
>
> It's a bit odd having `ret` be 1 at the outset rather than 0, thus
> making the logic a bit more difficult to reason about. I would have
> expected it to be organized like this:
>
> int ret = 0;
> if (refs_read_raw_ref(...)) {
> if (failure_errno == ENOENT) {
> ret = 2;
> } else {
> ret = 1;
> errno = failure_errno;
> error_errno(_("failed to look up reference: %s"));
> }
> }
> strbuf_release(...);
> return ret;
Fair enough. I've seen both styles used in our codebase, but ultimately
don't care much which of either we use here. Will adapt.
> > @@ -272,13 +309,15 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
> > + if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
> > + die(_("only one of --exclude-existing, --exists or --verify can be given"));
>
> When reviewing an earlier patch in this series, I forgot to mention
> that we can simplify the life of translators by using placeholders:
>
> die(_("options '%s', '%s' or '%s' cannot be used together"),
> "--exclude-existing", "--exists", "--verify");
>
> which ensures that they don't translate the literal option names, and
> makes it possible to reuse the translated message in multiple
> locations (since it doesn't mention hard-coded option names).
Done.
Thanks for your review, highly appreciated! I'll wait until tomorrow for
additional feedback and then send out v2.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 10/12] builtin/show-ref: explicitly spell out different modes in synopsis
From: Patrick Steinhardt @ 2023-10-25 11:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <CAPig+cQ=VhRm40oW=TQYzy2NXKNJm4QVQhtw3FAKsFRn12qYLA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3246 bytes --]
On Tue, Oct 24, 2023 at 03:39:28PM -0400, Eric Sunshine wrote:
> On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> > The synopsis treats the `--verify` and the implicit mode the same. They
> > are slightly different though:
> >
> > - They accept different sets of flags.
> >
> > - The implicit mode accepts patterns while the `--verify` mode
> > accepts references.
> >
> > Split up the synopsis for these two modes such that we can disambiguate
> > those differences.
>
> Good. When reading [2/12], my immediate thought was that such a
> documentation change was needed.
>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
> > @@ -8,9 +8,12 @@ git-show-ref - List references in a local repository
> > SYNOPSIS
> > -'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference]
> > +'git show-ref' [-q | --quiet] [--head] [-d | --dereference]
> > [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
> > [--heads] [--] [<pattern>...]
> > +'git show-ref' --verify [-q | --quiet] [-d | --dereference]
> > + [-s | --hash[=<n>]] [--abbrev[=<n>]]
> > + [--] [<ref>...]
> > 'git show-ref' --exclude-existing[=<pattern>]
>
> What does it mean to request "quiet" for the plain `git show-ref`
> mode? That seems pointless and counterintuitive. Even though this mode
> may accept --quiet as a quirk of implementation, we probably shouldn't
> be promoting its use in the documentation. Moreover, the blurb for
> --quiet later in the document:
>
> Do not print any results to stdout. When combined with --verify,
> this can be used to silently check if a reference exists.
>
> should probably be rephrased since it currently implies that it may be
> used with modes other than --verify, but that's not really the case
> (implementation quirks aside).
Good point indeed, will change.
> This also raises the question as to whether an interlock should be
> added to disallow --quiet with plain `git show-ref`, much like the
> interlock preventing --exclude-existing and --verify from being used
> together. Ideally, such an interlock ought to be added, but I wouldn't
> be surprised to learn that doing so would break someone's existing
> tooling which insensibly uses --quiet with plain `git show-ref`.
Yeah, I also wouldn't go as far as this. The mutual exclusiveness for
`--exclude-existing` and `--verify` makes sense in my opinion because
the result is extremely misleading and may cause users to assume that
the wrong thing has happened.
I don't think that's necessarily true for `--quiet`. It may not make a
lot of sense to specify `--quiet` here, but it also doesn't quietly do
the wrong thing as in the other case.
Furthermore, we also don't have any interlocks for incompatible other
flags, either: git-show-ref(1) won't complain when passing any of the
mode-specific flags to the other modes. If we want to fix that I'd
rather defer it to a follow up patch series though. And as you said, I
would almost certainly expect there to be some kind of fallout if we did
this change.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Patrick Steinhardt @ 2023-10-25 11:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Junio C Hamano, Han-Wen Nienhuys
In-Reply-To: <CAPig+cQrD6uh65UzaKbwryv=wcdymKrjqXsAKgrKHYpQNWqSYQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]
On Tue, Oct 24, 2023 at 02:48:14PM -0400, Eric Sunshine wrote:
> On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> wrote:
> > It's not immediately obvious options which options are applicable to
> > what subcommand ni git-show-ref(1) because all options exist as global
>
> s/ni/in/
>
> > state. This can easily cause confusion for the reader.
> >
> > Refactor options for the `--exclude-existing` subcommand to be contained
> > in a separate structure. This structure is stored on the stack and
> > passed down as required. Consequentially, it clearly delimits the scope
>
> s/Consequentially/Consequently/
>
> > of those options and requires the reader to worry less about global
> > state.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> > @@ -95,6 +94,11 @@ static int add_existing(const char *refname,
> > +struct exclude_existing_options {
> > + int enabled;
> > + const char *pattern;
> > +};
>
> Do we need this `enabled` flag? Can't the same be achieved by checking
> whether `pattern` is NULL or not (see below)?
Yeah, we do. It's perfectly valid to pass `--exclude-existing` without
the optional pattern argument. We still want to use this mode in that
case, but don't populate the pattern.
An alternative would be to assign something like a sentinel value in
here. But I'd think that it's clearer to instead have an explicit
separate field for this.
> > @@ -104,11 +108,11 @@ static int add_existing(const char *refname,
> > -static int cmd_show_ref__exclude_existing(const char *match)
> > +static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts)
>
> Since you're renaming `match` to `opts->pattern`...
>
> > {
> > - int matchlen = match ? strlen(match) : 0;
> > + int matchlen = opts->pattern ? strlen(opts->pattern) : 0;
>
> ... and since you're touching this line anyway, maybe it makes sense
> to rename `matchlen` to `patternlen`?
Yes, let's do it. It's been more of an oversight rather than
intentional to keep the previous name.
> > @@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match)
> > - if (strncmp(ref, match, matchlen))
> > + if (strncmp(ref, opts->pattern, matchlen))
>
> Especially since, as shown in this context, `matchlen` is really the
> length of the _pattern_, not the length of the resulting _match_.
>
> > @@ -200,44 +204,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset)
> > int cmd_show_ref(int argc, const char **argv, const char *prefix)
> > {
> > [...]
> > - if (exclude_arg)
> > - return cmd_show_ref__exclude_existing(exclude_existing_arg);
> > + if (exclude_existing_opts.enabled)
> > + return cmd_show_ref__exclude_existing(&exclude_existing_opts);
>
> (continued from above) Can't this be handled without a separate `enabled` flag?
>
> if (exclude_existing_opts.pattern)
> ...
See the explanation above.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v3] git-rebase.txt: rewrite docu for fixup/squash (again)
From: Oswald Buddenhagen @ 2023-10-25 10:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Phillip Wood, Taylor Blau, Christian Couder,
Charvi Mendiratta, Marc Branchaud
In-Reply-To: <20231023130016.1093356-1-oswald.buddenhagen@gmx.de>
Create a clear top-down structure which makes it hopefully unambiguous
what happens when.
The behavior in the presence of multiple "fixup -c" is somewhat
questionable, as arguably it would be better to complain about it rather
than letting the last instance win. But for the time being we document
the status quo, with a note that it is not guaranteed. Note that
actually changing it would require --autosquash eliding the superseded
uses.
Also emphasize that the author info of the first commit is preserved
even in the presence of "fixup -c", as this diverges from "git commit
-c"'s behavior. New options matching the latter should be introduced for
completeness.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v3:
- adjust to reality, and elaborate in the commit message why it's
arguably somewhat suboptimal
i deliberated the 'command "pick"' word order swap suggested by marc,
but while it improves things locally, it somehow doesn't flow with the
"redundancy-reduced" last part of the sentence.
v2:
- slight adjustments inspired by marc. however, i left most things
unchanged or even went in the opposite direction, because i assume the
readers to be sufficiently context-sensitive, and the objective is
merely to be not actively confusing. adding redundancy in the name of
clarity would just make the text stylistically inferior and arguably
harder to read.
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
Cc: Taylor Blau <me@ttaylorr.com>
Cc: Christian Couder <christian.couder@gmail.com>
Cc: Charvi Mendiratta <charvi077@gmail.com>
Cc: Marc Branchaud <marcnarc@xiplink.com>
---
Documentation/git-rebase.txt | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e7b39ad244..578d1d34a6 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -890,20 +890,22 @@ 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
+message of the first commit with those of commits identified by "squash"
+commands, omitting those 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 (if multiple are present, the last one
+takes precedence, but this should not be relied upon).
+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 author information (including date/timestamp) always comes from
+the first commit; this is the case even if "fixup -c/-C" is used,
+contrary to what `git commit` does.
`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
* Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option
From: Karthik Nayak @ 2023-10-25 9:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps
In-Reply-To: <xmqqbkcn52z4.fsf@gitster.g>
On Tue, Oct 24, 2023 at 7:45 PM Junio C Hamano <gitster@pobox.com> wrote:
> I would suspect that swapping if/else would make it easier to
> follow. Everybody else in the patch guards the use of the oidset
> with "were we told not to die on missing objects?", i.e.,
>
> if (revs->do_not_die_on_missing_objects)
> oidset_insert(&revs->missing_objects, &p->object.oid);
> else
> return -1; /* corrupt repository */
>
Makes sense. Will change.
> > @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)
> > FOR_EACH_OBJECT_PROMISOR_ONLY);
> > }
> >
> > + if (revs->do_not_die_on_missing_objects)
> > + oidset_init(&revs->missing_objects, 0);
>
> I read the patch to make sure that .missing_objects oidset is used
> only when .do_not_die_on_missing_objects is set and the oidset is
> untouched unless it is initialized. Well done.
>
> I know I floated "perhaps oidset can replace the object bits,
> especially because the number of objects that need marking is
> expected to be small", but I am curious what the performance
> implication of this would be. Is this something we can create
> comparison easily?
I did a simple comparision here, I randomly deleted commits which had
child commits with greater than 2 parents.
$ git rev-list --missing=print HEAD | grep "?" | wc -l
828
Using the flag bit:
$ hyperfine -w 3 "~/git/bin-wrappers/git rev-list --missing=allow-any HEAD"
Benchmark 1: ~/git/bin-wrappers/git rev-list --missing=allow-any HEAD
Time (mean ± σ): 860.5 ms ± 15.2 ms [User: 375.3 ms, System: 467.5 ms]
Range (min … max): 835.2 ms … 881.0 ms 10 runs
Using the oidset:
$ hyperfine -w 3 "~/git/bin-wrappers/git rev-list --missing=allow-any HEAD"
Benchmark 1: ~/git/bin-wrappers/git rev-list --missing=allow-any HEAD
Time (mean ± σ): 901.3 ms ± 9.6 ms [User: 394.3 ms, System: 488.3 ms]
Range (min … max): 885.0 ms … 913.2 ms 10 runs
Its definitely slower, but not by much.
>
> > I noticed that nobody releases the resource held by this new oidset.
> > Shouldn't we do so in revision.c:release_revisions()?
>
> It seems that linux-leaks CI job noticed the same.
>
> https://github.com/git/git/actions/runs/6633599458/job/18021612518#step:5:2949
>
> I wonder if the following is sufficient?
>
> revision.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git c/revision.c w/revision.c
> index 724a116401..7a67ff74dc 100644
> --- c/revision.c
> +++ w/revision.c
> @@ -3136,6 +3136,8 @@ void release_revisions(struct rev_info *revs)
> clear_decoration(&revs->merge_simplification, free);
> clear_decoration(&revs->treesame, free);
> line_log_free(revs);
> + if (revs->do_not_die_on_missing_objects)
> + oidset_clear(&revs->missing_objects);
> }
>
> static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
>
Yup, this seems right and was missed, will add.
On Wed, Oct 25, 2023 at 8:40 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> >
> > if (commit->object.flags & ADDED)
> > return 0;
> > + if (revs->do_not_die_on_missing_objects &&
> > + oidset_contains(&revs->missing_objects, &commit->object.oid))
>
> Nit: indentation is off here.
>
Thanks, will fix.
> > +
> > + /* Missing objects to be tracked without failing traversal. */
> > + struct oidset missing_objects;
>
> As far as I can see we only use this set to track missing commits, but
> none of the other objects. The name thus feels a bit misleading to me,
> as a reader might rightfully assume that it contains _all_ missing
> objects after the revwalk. Should we rename it to `missing_commits` to
> clarify?
>
Fair enough, I was thinking of being future compatible. But probably best to
be specific now and refactor as needed in the future. Will change.
Thanks both for the review!
^ permalink raw reply
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
From: Oswald Buddenhagen @ 2023-10-25 9:23 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
In-Reply-To: <20231025061120.GA2094463@coredump.intra.peff.net>
On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
>The "//" operator was added in perl 5.10. I'm not sure what you found
>that makes you think the ship has sailed. The only hits for "//" I see
>look like the end of substitution regexes ("s/foo//" and similar).
>
grep with spaces around the operator, then you can see the instance in
git-credential-netrc.perl easily.
regards
^ permalink raw reply
* [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Ruslan Yakauleu via GitGitGadget @ 2023-10-25 8:58 UTC (permalink / raw)
To: git; +Cc: Ruslan Yakauleu, Ruslan Yakauleu
From: Ruslan Yakauleu <ruslan.yakauleu@gmail.com>
A new option --ff-one-only to control the merging strategy.
For one commit option works like -ff to avoid extra merge commit.
In other cases the option works like --no-ff to create merge commit for
complex features.
Signed-off-by: Ruslan Yakauleu <ruslan.yakauleu@gmail.com>
---
merge: --ff-one-only to apply FF if commit is one
A new option --ff-one-only to control the merging strategy. For one
commit option works like -ff to avoid extra merge commit. In other cases
the option works like --no-ff to create merge commit for complex
features.
Plenty of developers want to simplify merge history. We have two main
merging strategies:
* Fast-forward (--ff) - There we can lose merge commits for complex
features and if we need to roll back some feature we can't revert
just one commit.
* Merge (--no-ff) - There we have extra merges for extra simple
changes.
Before, the user had to choose between --ff/--no-ff every time to have
history without extra merges for simple changes and to use merges for
complex features.
Ways to use the new option: git merge --ff-one-only git config merge.ff
one-only git config branch.master.mergeoptions --ff-one-only
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1599%2FQuAzI%2Fmerge%2Fff-one-only-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1599/QuAzI/merge/ff-one-only-v1
Pull-Request: https://github.com/git/git/pull/1599
Documentation/config/merge.txt | 3 +++
builtin/merge.c | 18 +++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt
index 8851b6cedef..6cd5daa4d64 100644
--- a/Documentation/config/merge.txt
+++ b/Documentation/config/merge.txt
@@ -31,6 +31,9 @@ merge.ff::
a case (equivalent to giving the `--no-ff` option from the command
line). When set to `only`, only such fast-forward merges are
allowed (equivalent to giving the `--ff-only` option from the
+ command line). When set to `one-only`, fast-forward merge allowed
+ only for one commit, in other way extra merge commit should be
+ created (equivalent to giving the `--ff-one-only` option from the
command line).
merge.verifySignatures::
diff --git a/builtin/merge.c b/builtin/merge.c
index d748d46e135..100f0021e56 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -110,7 +110,8 @@ static const char *pull_twohead, *pull_octopus;
enum ff_type {
FF_NO,
FF_ALLOW,
- FF_ONLY
+ FF_ONLY,
+ FF_ONE_ONLY
};
static enum ff_type fast_forward = FF_ALLOW;
@@ -258,6 +259,7 @@ static struct option builtin_merge_options[] = {
N_("edit message before committing")),
OPT_CLEANUP(&cleanup_arg),
OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
+ OPT_SET_INT(0, "ff-one-only", &fast_forward, N_("allow fast-forward if only one commit"), FF_ONE_ONLY),
OPT_SET_INT_F(0, "ff-only", &fast_forward,
N_("abort if fast-forward is not possible"),
FF_ONLY, PARSE_OPT_NONEG),
@@ -631,6 +633,8 @@ static int git_merge_config(const char *k, const char *v,
fast_forward = boolval ? FF_ALLOW : FF_NO;
} else if (v && !strcmp(v, "only")) {
fast_forward = FF_ONLY;
+ } else if (v && !strcmp(v, "one-only")) {
+ fast_forward = FF_ONE_ONLY;
} /* do not barf on values from future versions of git */
return 0;
} else if (!strcmp(k, "merge.defaulttoupstream")) {
@@ -1527,6 +1531,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
free(list);
}
+ if (fast_forward == FF_ONE_ONLY) {
+ fast_forward = FF_NO;
+
+ /* check that we have one and only one commit to merge */
+ if (squash || ((!remoteheads->next &&
+ !common->next &&
+ oideq(&common->item->object.oid, &head_commit->object.oid)) &&
+ oideq(&remoteheads->item->parents->item->object.oid, &head_commit->object.oid))) {
+ fast_forward = FF_ALLOW;
+ }
+ }
+
update_ref("updating ORIG_HEAD", "ORIG_HEAD",
&head_commit->object.oid, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
base-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14
--
gitgitgadget
^ permalink raw reply related
* Re: [RFC PATCH 0/2] pretty: add %I formatting for patch-id
From: Jeff King @ 2023-10-25 8:44 UTC (permalink / raw)
To: Michael McClimon; +Cc: git
In-Reply-To: <20231022022800.69219-1-michael@mcclimon.org>
On Sat, Oct 21, 2023 at 10:27:58PM -0400, Michael McClimon wrote:
> I would like to have a single-command way to get the patch id for a
> commit: the thing you'd see in a pipeline like
> git diff-tree --patch-with-raw HEAD | git patch-id
So I actually don't think this is the worst thing in the world. You are
using two plumbing commands as intended. And if you want to get N
patch-ids, you can do it with a constant number of processes like:
git rev-list HEAD | git diff-tree --stdin -p | git patch-id
Alternatively, I wonder if patch-id could generate the ids internally
(which we certainly have code for; that's how git-cherry, etc, work).
Would it make sense to teach patch-id:
1. To accept just commit id's on stdin to produce the patches for
those commit id's against their parents, without receiving the
patch text itself on stdin. I guess --commits-only or something.
2. To accept commit-ishes on the command line, overriding the need to
read from stdin.
Which would let your single-patch case become just:
git patch-id --commits-only HEAD
But let's read on...
> My initial thought was to add a --patch-id flag to git diff-tree, but
> then I thought that maybe better would be to add a pretty specifier to
> do so, so that (for instance) you could generate patch-ids for
> everything in a branch by saying something like
> git log --pretty='%I %H' start..
I can see how "%I" would be convenient, especially from a scripting
standpoint, since you can control how it appears in the output. But it
does feel a little funny to me as a commit pretty-printing item.
Computing it requires calculating the diff, which we'll of course want
to do separately for the diff portion of many invocations. E.g., I
didn't check, but I'd guess that:
git log --pretty="%I %H" -p
would compute each diff twice. Would it make more sense as a
diff-format alongside --raw, --name-status, and so on?
I have a more subtle reason to think this might be a good idea, which
I'll discuss below.
> The thing that is perplexing to me is that it _does_ appear to work on
> some commits, for example (where 8b3aa36f is a recent-ish commit from
> master, chosen at random):
> [...]
> But for other commits, like the one in the test here, it does not.
Hmm. When I run the failing test in a debugger, I see that going into
commit_patch_id(), there is already an entry in diff_queued_diff (which
is a global that stores the result of the last diff). So I think there
is already a diff there from the call to diff_tree_oid() in
log_tree_diff(). And then you diff _again_ and end up with duplicates
when computing the patch-id.
(This causes another bug, too; at the end of diff_flush_patch_id we
clear the queue, so the actual diff is not showed by "git show" anymore;
the use of "-s" in the test obscures that).
The thing that most surprises me is that it ever works at all. ;) But I
think what happens is this:
- in the code in your patch, you fail to set up the diff_options
correctly. In particular, you do not set the "recursive" flag.
- so in the case of 8b3aa36f, which touches only a file in
Documentation/, the call from log_tree_diff() queues the correct
file modification entry. The second call from commit_patch_id()
mistakenly queues only the parent tree "Documentation"
- a patch-id is defined on the textual patch changes, so it ignores
non-file changes that are queued (and in Git you wouldn't normally
see these anyway unless you use a non-recursive diff or a special
option like "-t").
And hence any patch which consists of changes to files only in subtrees
will "work", and other ones fail. To "fix" your patch, you could set
diffopt.flags.recursive, as we do in format-patch's prepare_bases()
call. But of course because of the duplicate diff, that just means we'll
consistently get the wrong answer now. ;)
So I'd expect something like this to work:
diff --git a/pretty.c b/pretty.c
index 47e2e6e99a..5a473c7941 100644
--- a/pretty.c
+++ b/pretty.c
@@ -20,6 +20,7 @@
#include "run-command.h"
#include "object-name.h"
#include "patch-ids.h"
+#include "diffcore.h"
/*
* The limit for formatting directives, which enable the caller to append
@@ -1576,10 +1577,28 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
{
struct diff_options diffopt;
struct object_id patch_id;
+
+ /*
+ * save the existing diff queue, as it may be
+ * populated, e.g. by log_tree_diff(). Of course
+ * it would be nice to use its results, but:
+ *
+ * 1. We can't count on it (we might be formatting a
+ * via some other code path).
+ * 2. We have no clue which diff options were used.
+ */
+ struct diff_queue_struct old_queue = diff_queued_diff;
+ DIFF_QUEUE_CLEAR(&diff_queued_diff);
+
repo_diff_setup(the_repository, &diffopt);
+ diffopt.flags.recursive = 1;
+ diff_setup_done(&diffopt);
+
if (commit_patch_id(commit, &diffopt, &patch_id, 0))
die(_("cannot get patch id"));
strbuf_addstr(sb, oid_to_hex(&patch_id));
+
+ diff_queued_diff = old_queue;
return 1;
}
case 'm': /* left/right/bottom */
but it doesn't quite fix your test. There I think the issue is that
the patch-id code is a bit wonky for the special case of the creation of
an empty file (even before your patch). The internal patch-id generator
adds fake:
--- /dev/null
+++ b/whatever
lines that would normally appear in the patch for a newly created file.
But for an empty file, the "--patch" format does not emit those lines at
all! So we need this hackery on top:
diff --git a/diff.c b/diff.c
index 2c602df10a..9370e585f1 100644
--- a/diff.c
+++ b/diff.c
@@ -6431,15 +6431,15 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
the_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),
the_hash_algo->hexsz);
} else {
- if (p->one->mode == 0) {
+ if (p->one->mode == 0 && p->two->size) {
patch_id_add_string(&ctx, "---/dev/null");
patch_id_add_string(&ctx, "+++b/");
the_hash_algo->update_fn(&ctx, p->two->path, len2);
- } else if (p->two->mode == 0) {
+ } else if (p->two->mode == 0 && p->one->size) {
patch_id_add_string(&ctx, "---a/");
the_hash_algo->update_fn(&ctx, p->one->path, len1);
patch_id_add_string(&ctx, "+++/dev/null");
- } else {
+ } else if (p->one->size || p->two->size) {
patch_id_add_string(&ctx, "---a/");
the_hash_algo->update_fn(&ctx, p->one->path, len1);
patch_id_add_string(&ctx, "+++b/");
And with that, your new test succeeds.
> have done a bit of investigation, but I would not really call myself a C
> programmer and I'm not super familiar with the codebase, so I'm a bit
> stuck. I thought maybe at first I wasn't initializing the diff_options
> correctly, but I suspect the problem is actually more fundamental than
> that.
So as you can see, the diff options do matter. And that is a more
fundamental issue. In many cases patch-id will be pretty stable and
independent of options, but I suspect it would get confused by things
like rename detection.
If we run "git log --format=%I", which diff options should be used for
the patch-id? A stable set (like we do for "format-patch --base")? Or
ones that match the diff we are otherwise generating? I could see
arguments both ways.
One nice thing about "diff-tree | patch-id" is that you are in control
of the options that diff-tree uses. But I'd think in most cases you just
want a stable patch-id, so the "git patch-id HEAD" I suggested earlier
would presumably use some very basic set of diff options.
I dunno. I do not use patch-id much (aside from internal use via things
like "git cherry", etc). So there may be use cases or corner cases I'm
not thinking about. But I do think there's a bit of complexity to think
about here.
-Peff
^ permalink raw reply related
* Re: [PATCH 2/2] fetch: no redundant error message for atomic fetch
From: Patrick Steinhardt @ 2023-10-25 8:21 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <CANYiYbG0YFc4Hg=e+0db4NBgM2QwOLpjHjfp8WaoObNxR-=euA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]
On Tue, Oct 24, 2023 at 07:20:08AM +0800, Jiang Xin wrote:
> On Mon, Oct 23, 2023 at 6:07 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Oct 23, 2023 at 05:16:20PM +0800, Jiang Xin wrote:
> > > On Mon, Oct 23, 2023 at 4:27 PM Patrick Steinhardt <ps@pks.im> wrote:
> > > >
> > > > On Thu, Oct 19, 2023 at 10:34:33PM +0800, Jiang Xin wrote:
> > > > > @@ -1775,10 +1775,8 @@ static int do_fetch(struct transport *transport,
> > > > > }
> > > > >
> > > > > cleanup:
> > > > > - if (retcode && transaction) {
> > > > > - ref_transaction_abort(transaction, &err);
> > > > > + if (retcode && transaction && ref_transaction_abort(transaction, &err))
> > > > > error("%s", err.buf);
> > > > > - }
> > > >
> > > > Right. We already call `error()` in all cases where `err` was populated
> > > > before we `goto cleanup;`, so calling it unconditionally a second time
> > > > here is wrong.
> > > >
> > > > That being said, `ref_transaction_abort()` will end up calling the
> > > > respective backend's implementation of `transaction_abort`, and for the
> > > > files backend it actually ignores `err` completely. So if the abort
> > > > fails, we would still end up calling `error()` with an empty string.
> > >
> > > The transaction_abort implementations of the two builtin refs backends
> > > will not use "err“ because they never fail (always return 0). Some one
> > > may want to implement their own refs backend which may use the "err"
> > > variable in their "transaction_abort". So follow the pattern as
> > > update-ref.c and files-backend.c to call ref_transaction_abort() is
> > > safe.
> > >
> > > > Furthermore, it can happen that `transaction_commit` fails, writes to
> > > > the buffer and then prints the error. If the abort now fails as well, we
> > > > would end up printing the error message twice.
> > >
> > > The abort never fails so error message from transaction_commit() will
> > > not reach the code.
> >
> > With that reasoning we could get rid of the error handling of abort
> > completely as it's known not to fail. But only because it does not fail
> > right now doesn't mean that it won't in the future, as the infra for it
> > to fail is all in place. And in case it ever does the current code will
> > run into the bug I described.
>
> If in the future ref_transaction_abort() fails for some reason, the
> err variable will be filled with the error message and the previous
> error message will be discarded, no duplication will occur. So I think
> use this fix is OK.
Isn't that assuming quite a lot about that future code though? It
assumes both:
- That the code knows to always populate the error in the first
place. Otherwise we may end up with the same empty error message
that you aim to fix.
- That the code will know to overwrite it instead of appending to
it. Otherwise we may end up printing previous errors a second
time.
Both of these assumptions may not hold. Current code that does write to
the error buffer for example will always append to it, not overwrite its
preexisting contents. And it's likely that future code will do the same.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Christian Couder @ 2023-10-25 8:18 UTC (permalink / raw)
To: Achu Luma; +Cc: git, Junio C Hamano
In-Reply-To: <CAFR+8DzdFbwaiHtZSdLMqWYWh=fK0WA4c48+eBug-ZeAgddhcQ@mail.gmail.com>
On Tue, Oct 24, 2023 at 4:25 PM Achu Luma <ach.lumap@gmail.com> wrote:
>
> On Mon, Oct 23, 2023 at 2:41 PM Christian Couder
> <christian.couder@gmail.com> wrote:
> > Maybe if you have time you could add some descriptions or comments
> > related to the above emails and documents. For example you could tell
> > what the new unit test framework will be like, how the unit tests will
> > look like, etc. Maybe a short overview would be nice.
> >
> sure,
> 1- https://lore.kernel.org/git/0169ce6fb9ccafc089b74ae406db0d1a8ff8ac65.1688165272.git.steadmon@google.com/
> :
> The emails highlight the significant milestones achieved in
> defining and testing the custom TAP
> framework for writing git unit tests. It also contains some
> examples of implementation such as
> that of STRBUF_INIT with output:
> ok 1 - static initialization works
> 1..1
>
> 2- https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc:
> From this technical doc, the new unit test framework in the Git
> project represents a significant
> enhancement, introducing a systematic and efficient approach to
> unit testing. The custom git
> TAP implementation was selected from several alternatives based
> on strict criteria as the most
> suitable test framework for porting the unit tests.
> The unit tests are written in pure C, eliminating the need for
> the previous shell/test-tool helper
> setup, simplifying test configuration, data handling, and
> reducing testing runtime.
> Each unit test is encapsulated as a function and employs a range
> of predefined check functions
> for validation. These checks can evaluate conditions, compare
> integers or characters, validate
> strings, and more, providing comprehensive coverage for test scenarios.
>
> When a test is run using the TEST() macro, it undergoes a series
> of checks, and if any check fails,
> a diagnostic message is printed to aid in debugging. This
> diagnostic output includes information
> about the specific check that failed, the file and line number
> where it occurred, and a clear comparison
> of the expected and actual values. Such detailed reporting
> simplifies the identification and resolution
> of issues, contributing to codebase stability.
>
> Additionally, the framework supports features like skipping tests
> with explanations, sending custom
> diagnostic messages using test_msg(), and marking known-to-fail
> checks using TEST_TODO().
> This flexibility allows developers to tailor their tests to
> specific scenarios while ensuring a
> comprehensive testing suite.
Ok, please add all these explanations above as well as those below to
your application document. We prefer that you consider your
application document like a patch. So you would send to the mailing
list several versions of it for review before submitting officially.
> > You could also try to apply the patches in the series that adds the
> > test framework, or alternatively use the 'seen' branch where the
> > series has been merged, and start playing with it by writing, or
> > porting, a small example test.
> >
> ok, I think I can push a patch for one.
I don't think it's necessary to send it to the mailing list for now,
but it should definitely be part of your application document.
> > > -- 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.
> >
> > Could you provide an example of what the new name would be for an
> > existing test that is worth porting?
> >
> Sure... let's consider an existing unit test in t/helper directory
> such as t/helper/test-date.c or
> its shell named t0006-date.sh, which is part of the current
> shell-based test suite. In the context
> of the new unit testing framework, this test could be reimagined and renamed as
> "t-date.c". The "t-" prefix is typically used for test program files
> in Git, and "date" is retained to
> reflect the nature of the tests within this suite.
Ok.
> > > -- 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"
> >
> > Maybe you could continue the above example and tell which headers
> > would be needed for it?
> >
> > > -- 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...
> > > }");
> >
> > Ok, a simple example would be nice too.
> >
> we can continue with the example used for naming: test-date.c. a
> typical t-date.c unit test would look
> like the following:
> --
> #include "test-lib.h"
> #include "date.h"
> --
> date.h here is a necessary header file. Now refactoring the test logic
> from the original shell script:
> --
> #include "test-lib.h"
> #include "date.h"
>
> static void test_parse_dates(void)
> {
> const char *dates[] = { "invalid_date", "2023-10-17 10:00:00 +0200", NULL };
>
> for (const char **argv = dates; *argv; argv++) {
> check_int(parse_dates((const char *[]){ *argv, NULL }), 0);
> }
> }
> --
Nice!
> > > -- 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.
> >
> > This would seem to be part of the previous step, as you would have to
> > provide a description when using the testing macro. But Ok.
> >
> > > -- 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();
> > > }
> >
> > Yeah, continuing an example would be nice.
> >
> Continuing, we can add a test entrance as follows:
> --
> #include "test-lib.h"
> #include "date.h"
>
> static void test_parse_dates(void)
> {
> const char *dates[] = { "invalid_date", "2023-10-17 10:00:00 +0200", NULL };
>
> for (const char **argv = dates; *argv; argv++) {
> check_int(parse_dates((const char *[]){ *argv, NULL }), 0);
> }
> }
>
> int main(int argc UNUSED, const char **argv UNUSED)
> {
> TEST(test_parse_dates, "Test date parsing");
>
> return test_done();
> }
> --
>
> A typical unit tests with the custom TAP framework would look
> something like above. This might run in theory
> but I have not yet run it as I used it here just for demonstration.
> The unit tests can be built using
> "make unit-tests." Additionally, Makefile can be modified to add the
> file to the build:
> --
> UNIT_TEST_PROGRAMS += t-date
> --
Great!
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Patrick Steinhardt @ 2023-10-25 8:05 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Taylor Blau, git, stolee
In-Reply-To: <2b764f52-d3ae-467f-a915-fb73beb247bb@app.fastmail.com>
[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]
On Wed, Oct 25, 2023 at 09:33:23AM +0200, Kristoffer Haugsbakk wrote:
> On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote:
> >> What do you guys think the signature of `git_global_config` should be?
> >
> > Either of the following:
> >
> > - `int git_global_config(char **out_pat)`
> > - `char **git_global_config(void)`
> >
> > In the first case you'd signal error via a non-zero return value,
> > whereas in the second case you would signal it via a `NULL` return
> > value.
> >
> > To decide which one to go with I'd recommend to check whether there is
> > any similar precedent in "config.h" and what style that precedent uses.
>
> Okay thanks. So no parameter for determining whether one is writing or
> just reading the file.
This parameter would only exist for the purpose of the error message,
right? If so, I think that'd be overkill. If we want to have differing
errors depending on how the function is called the best way to handle
that would likely be to generate the error message at the callsite
instead of in the library itself.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [OUTREACHY] Final Application For Git Internshhip
From: Christian Couder @ 2023-10-25 8:03 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2yJa3xvCPN6bqsa4+vSkwsdUouhqNvuH6y_CC2cJ0YSmQ@mail.gmail.com>
On Tue, Oct 24, 2023 at 9:48 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> I'm Ibe Naomi Amarachi,I'm a Nigerian and I currently live in
> Lagos,Nigeria. I'm a graduate of the African Leadership Xcelerator
> Software Engineering program (ALX SE), with specialization in backend
> web development and I'm applying for the "Moving Existing Tests to a
> Unit Testing Framework" project
>
> Some of my projects that involve working with Shell,C and Git can be found here:
>
> https://github.com/Amarajah/alx-system_engineering-devops
>
> https://github.com/Amarajah/alx-low_level_programming
>
> Git has been a part of my software engineering journey from day one
> and it's allowed me to collaborate with peers and also keep track of
> my personal projects. Currently Git uses end-to-end tests for error
> conditions that could easily be captured by unit tests, the project is
> aimed at turning end-to-end tests to unit tests, and I'd love to be a
> part of it
Maybe a period is missing at the end of the above sentence. Otherwise Ok.
> My microproject contribution is here:
>
> https://public-inbox.org/git/20231009011652.1791-1-naomi.ibeh69@gmail.com/T/#u
>
> And here is my updated contribution link after review by the Git community:
>
> https://public-inbox.org/git/xmqqttqox5cp.fsf@gitster.g/T/#u
It could help to say if your contribution has been merged to 'master',
'next', 'seen' or not at all.
> Below is my project timeline:
> (Of course I'd be very much willing to work with the community and
> mentors to edit it so it perfectly meets up to the community's
> expectations)
>
> October 2, 2023 - October 30, 2023
>
> Familiarizing myself with the community , mailing list and
> contributing my microproject
>
> November 20, 2023 - December 4, 2023
>
> Familiarizing myself with the already existing tests and also the
> chosen unit test framework
>
> Do more research on the internship projects and find better ways to
> get it done in harmony with coding guidelines and community
> requirements
>
> December 4, 2023 - December 31, 2023
>
> First document the initial state of the test files, then make sure all
> test files conform to coding guidelines down to the tiniest details
> (e.g git/t/helper/test-write-cache.c and git/t/helper/test-advise.c
> have die() messages which do not conform to coding guidelines)
We don't advise spending a lot of time during your internship with
small things that could be part of someone else's microproject later.
You should be focused on the internship goal first.
Of course if you are migrating some code to the new unit test
framework, it's Ok to improve that code before migrating it. But no
need to improve everything under t/helper before starting to migrate
parts of the code there.
I think that one of the important tasks to be done early is to
identify what code in t/helper is unit testing C code and what code is
really about helping other tests in the t/t*.sh scripts. It would be
nice if you could give an example of each kind of code.
> January 2, 2024 - January 31, 2024
>
> Run the tests and verify they still work as originally intended
>
> Begin migrations of test files
>
> Test migrated files and make necessary changes based on feedback
> received from teammates and mentors
An example of how you would migrate parts of a test, or how a migrated
test would look like, would be nice.
> February 1, 2024 - March 1, 2024
>
> Continue testing migrated files and making necessary changes based on
> feedback received from teammates and mentors
>
> Document each step and request for reviews from teammates and mentors
>
> Tidy up the project,make sure all necessary files are migrated, they
> all work as intended, they are well documented and that there are no
> conflicts
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH v5 0/5] merge-ort: implement support for packing objects together
From: Patrick Steinhardt @ 2023-10-25 7:58 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1698101088.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 18764 bytes --]
On Mon, Oct 23, 2023 at 06:44:49PM -0400, Taylor Blau wrote:
> (Rebased onto the current tip of 'master', which is ceadf0f3cf (The
> twentieth batch, 2023-10-20)).
>
> This series implements support for a new merge-tree option,
> `--write-pack`, which causes any newly-written objects to be packed
> together instead of being stored individually as loose.
>
> This is a minor follow-up that could be taken instead of v4 (though the
> changes between these two most recent rounds are stylistic and a matter
> of subjective opinion).
>
> This moves us the bulk_checkin_source structure introduced in response
> to Junio's suggestion during the last round further in the OOP
> direction. Instead of switching on the enum type of the source, have
> function pointers for read() and seek() respectively.
>
> The functionality at the end is the same, but this avoids some of the
> namespacing issues that Peff pointed out while looking at v4. But I
> think that this approach ended up being less heavy-weight than I had
> originally imagined, so I think that this version is a worthwhile
> improvement over v4.
>
> Beyond that, the changes since last time can be viewed in the range-diff
> below. Thanks in advance for any review!
Overall this version looks good to me. I've only got two smallish nits
and one question regarding the tests.
Thanks!
Patrick
> [1]: https://lore.kernel.org/git/xmqq34y7plj4.fsf@gitster.g/
>
> Taylor Blau (5):
> bulk-checkin: extract abstract `bulk_checkin_source`
> bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
> bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
> bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
> builtin/merge-tree.c: implement support for `--write-pack`
>
> Documentation/git-merge-tree.txt | 4 +
> builtin/merge-tree.c | 5 +
> bulk-checkin.c | 197 +++++++++++++++++++++++++++----
> bulk-checkin.h | 8 ++
> merge-ort.c | 42 +++++--
> merge-recursive.h | 1 +
> t/t4301-merge-tree-write-tree.sh | 93 +++++++++++++++
> 7 files changed, 316 insertions(+), 34 deletions(-)
>
> Range-diff against v4:
> 1: 97bb6e9f59 ! 1: 696aa027e4 bulk-checkin: extract abstract `bulk_checkin_source`
> @@ bulk-checkin.c: static int already_written(struct bulk_checkin_packfile *state,
> }
>
> +struct bulk_checkin_source {
> -+ enum { SOURCE_FILE } type;
> ++ off_t (*read)(struct bulk_checkin_source *, void *, size_t);
> ++ off_t (*seek)(struct bulk_checkin_source *, off_t);
> +
> -+ /* SOURCE_FILE fields */
> -+ int fd;
> ++ union {
> ++ struct {
> ++ int fd;
> ++ } from_fd;
> ++ } data;
> +
> -+ /* common fields */
> + size_t size;
> + const char *path;
> +};
> +
> -+static off_t bulk_checkin_source_seek_to(struct bulk_checkin_source *source,
> -+ off_t offset)
> ++static off_t bulk_checkin_source_read_from_fd(struct bulk_checkin_source *source,
> ++ void *buf, size_t nr)
> +{
> -+ switch (source->type) {
> -+ case SOURCE_FILE:
> -+ return lseek(source->fd, offset, SEEK_SET);
> -+ default:
> -+ BUG("unknown bulk-checkin source: %d", source->type);
> -+ }
> ++ return read_in_full(source->data.from_fd.fd, buf, nr);
> +}
> +
> -+static ssize_t bulk_checkin_source_read(struct bulk_checkin_source *source,
> -+ void *buf, size_t nr)
> ++static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source,
> ++ off_t offset)
> +{
> -+ switch (source->type) {
> -+ case SOURCE_FILE:
> -+ return read_in_full(source->fd, buf, nr);
> -+ default:
> -+ BUG("unknown bulk-checkin source: %d", source->type);
> -+ }
> ++ return lseek(source->data.from_fd.fd, offset, SEEK_SET);
> ++}
> ++
> ++static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> ++ int fd, size_t size,
> ++ const char *path)
> ++{
> ++ memset(source, 0, sizeof(struct bulk_checkin_source));
> ++
> ++ source->read = bulk_checkin_source_read_from_fd;
> ++ source->seek = bulk_checkin_source_seek_from_fd;
> ++
> ++ source->data.from_fd.fd = fd;
> ++
> ++ source->size = size;
> ++ source->path = path;
> +}
> +
> /*
> @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *sta
> - ssize_t read_result = read_in_full(fd, ibuf, rsize);
> + ssize_t read_result;
> +
> -+ read_result = bulk_checkin_source_read(source, ibuf,
> -+ rsize);
> ++ read_result = source->read(source, ibuf, rsize);
> if (read_result < 0)
> - die_errno("failed to read from '%s'", path);
> + die_errno("failed to read from '%s'",
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> unsigned header_len;
> struct hashfile_checkpoint checkpoint = {0};
> struct pack_idx_entry *idx = NULL;
> -+ struct bulk_checkin_source source = {
> -+ .type = SOURCE_FILE,
> -+ .fd = fd,
> -+ .size = size,
> -+ .path = path,
> -+ };
> ++ struct bulk_checkin_source source;
> ++
> ++ init_bulk_checkin_source_from_fd(&source, fd, size, path);
>
> seekback = lseek(fd, 0, SEEK_CUR);
> if (seekback == (off_t) -1)
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> state->offset = checkpoint.offset;
> flush_bulk_checkin_packfile(state);
> - if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
> -+ if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
> ++ if (source.seek(&source, seekback) == (off_t)-1)
> return error("cannot seek back");
> }
> the_hash_algo->final_oid_fn(result_oid, &ctx);
> 2: 9d633df339 < -: ---------- bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
> 3: d5bbd7810e ! 2: 596bd028a7 bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`
> @@ Metadata
> Author: Taylor Blau <me@ttaylorr.com>
>
> ## Commit message ##
> - bulk-checkin: refactor deflate routine to accept a `bulk_checkin_source`
> + bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
>
> - Prepare for a future change where we will want to use a routine very
> - similar to the existing `deflate_blob_to_pack()` but over arbitrary
> - sources (i.e. either open file-descriptors, or a location in memory).
> + The existing `stream_blob_to_pack()` function is named based on the fact
> + that it knows only how to stream blobs into a bulk-checkin pack.
>
> - Extract out a common "deflate_obj_to_pack()" routine that acts on a
> - bulk_checkin_source, instead of a (int, size_t) pair. Then rewrite
> - `deflate_blob_to_pack()` in terms of it.
> + But there is no longer anything in this function which prevents us from
> + writing objects of arbitrary types to the bulk-checkin pack. Prepare to
> + write OBJ_TREEs by removing this assumption, adding an `enum
> + object_type` parameter to this function's argument list, and renaming it
> + to `stream_obj_to_pack()` accordingly.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>
> ## bulk-checkin.c ##
> +@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> + * status before calling us just in case we ask it to call us again
> + * with a new pack.
> + */
> +-static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
> +- git_hash_ctx *ctx, off_t *already_hashed_to,
> +- struct bulk_checkin_source *source,
> +- unsigned flags)
> ++static int stream_obj_to_pack(struct bulk_checkin_packfile *state,
> ++ git_hash_ctx *ctx, off_t *already_hashed_to,
> ++ struct bulk_checkin_source *source,
> ++ enum object_type type, unsigned flags)
> + {
> + git_zstream s;
> + unsigned char ibuf[16384];
> +@@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
> +
> + git_deflate_init(&s, pack_compression_level);
> +
> +- hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB,
> +- size);
> ++ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
> + s.next_out = obuf + hdrlen;
> + s.avail_out = sizeof(obuf) - hdrlen;
> +
> @@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *state,
> die_errno("unable to write pack header");
> }
> @@ bulk-checkin.c: static void prepare_to_stream(struct bulk_checkin_packfile *stat
> unsigned header_len;
> struct hashfile_checkpoint checkpoint = {0};
> struct pack_idx_entry *idx = NULL;
> -- struct bulk_checkin_source source = {
> -- .type = SOURCE_FILE,
> -- .fd = fd,
> -- .size = size,
> -- .path = path,
> -- };
> +- struct bulk_checkin_source source;
>
> +- init_bulk_checkin_source_from_fd(&source, fd, size, path);
> +-
> - seekback = lseek(fd, 0, SEEK_CUR);
> - if (seekback == (off_t) -1)
> - return error("cannot find the current offset");
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> prepare_to_stream(state, flags);
> if (idx) {
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
> + idx->offset = state->offset;
> crc32_begin(state->f);
> }
> - if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
> -- &source, OBJ_BLOB, flags))
> +- if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
> +- &source, flags))
> ++ if (!stream_obj_to_pack(state, &ctx, &already_hashed_to,
> + source, type, flags))
> break;
> /*
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> hashfile_truncate(state->f, &checkpoint);
> state->offset = checkpoint.offset;
> flush_bulk_checkin_packfile(state);
> -- if (bulk_checkin_source_seek_to(&source, seekback) == (off_t)-1)
> -+ if (bulk_checkin_source_seek_to(source, seekback) == (off_t)-1)
> +- if (source.seek(&source, seekback) == (off_t)-1)
> ++ if (source->seek(source, seekback) == (off_t)-1)
> return error("cannot seek back");
> }
> the_hash_algo->final_oid_fn(result_oid, &ctx);
> @@ bulk-checkin.c: static int deflate_blob_to_pack(struct bulk_checkin_packfile *st
> + int fd, size_t size,
> + const char *path, unsigned flags)
> +{
> -+ struct bulk_checkin_source source = {
> -+ .type = SOURCE_FILE,
> -+ .fd = fd,
> -+ .size = size,
> -+ .path = path,
> -+ };
> -+ off_t seekback = lseek(fd, 0, SEEK_CUR);
> ++ struct bulk_checkin_source source;
> ++ off_t seekback;
> ++
> ++ init_bulk_checkin_source_from_fd(&source, fd, size, path);
> ++
> ++ seekback = lseek(fd, 0, SEEK_CUR);
> + if (seekback == (off_t) -1)
> + return error("cannot find the current offset");
> +
> 4: e427fe6ad3 < -: ---------- bulk-checkin: implement `SOURCE_INCORE` mode for `bulk_checkin_source`
> 5: 48095afe80 ! 3: d8cf8e4395 bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
> @@ Metadata
> ## Commit message ##
> bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
>
> - Now that we have factored out many of the common routines necessary to
> - index a new object into a pack created by the bulk-checkin machinery, we
> - can introduce a variant of `index_blob_bulk_checkin()` that acts on
> - blobs whose contents we can fit in memory.
> + Introduce `index_blob_bulk_checkin_incore()` which allows streaming
> + arbitrary blob contents from memory into the bulk-checkin pack.
> +
> + In order to support streaming from a location in memory, we must
> + implement a new kind of bulk_checkin_source that does just that. These
> + implementation in spread out across:
> +
> + - init_bulk_checkin_source_incore()
> + - bulk_checkin_source_read_incore()
> + - bulk_checkin_source_seek_incore()
> +
> + Note that, unlike file descriptors, which manage their own offset
> + internally, we have to keep track of how many bytes we've read out of
> + the buffer, and make sure we don't read past the end of the buffer.
>
> This will be useful in a couple of more commits in order to provide the
> `merge-tree` builtin with a mechanism to create a new pack containing
> @@ Commit message
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>
> ## bulk-checkin.c ##
> +@@ bulk-checkin.c: struct bulk_checkin_source {
> + struct {
> + int fd;
> + } from_fd;
> ++ struct {
> ++ const void *buf;
> ++ size_t nr_read;
> ++ } incore;
> + } data;
> +
> + size_t size;
> +@@ bulk-checkin.c: static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source
> + return lseek(source->data.from_fd.fd, offset, SEEK_SET);
> + }
> +
> ++static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,
> ++ void *buf, size_t nr)
> ++{
> ++ const unsigned char *src = source->data.incore.buf;
> ++
> ++ if (source->data.incore.nr_read > source->size)
> ++ BUG("read beyond bulk-checkin source buffer end "
> ++ "(%"PRIuMAX" > %"PRIuMAX")",
> ++ (uintmax_t)source->data.incore.nr_read,
> ++ (uintmax_t)source->size);
> ++
> ++ if (nr > source->size - source->data.incore.nr_read)
> ++ nr = source->size - source->data.incore.nr_read;
> ++
> ++ src += source->data.incore.nr_read;
> ++
> ++ memcpy(buf, src, nr);
> ++ source->data.incore.nr_read += nr;
> ++ return nr;
> ++}
> ++
> ++static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
> ++ off_t offset)
> ++{
> ++ if (!(0 <= offset && offset < source->size))
> ++ return (off_t)-1;
> ++ source->data.incore.nr_read = offset;
> ++ return source->data.incore.nr_read;
> ++}
> ++
> + static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> + int fd, size_t size,
> + const char *path)
> +@@ bulk-checkin.c: static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> + source->path = path;
> + }
> +
> ++static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,
> ++ const void *buf, size_t size,
> ++ const char *path)
> ++{
> ++ memset(source, 0, sizeof(struct bulk_checkin_source));
> ++
> ++ source->read = bulk_checkin_source_read_incore;
> ++ source->seek = bulk_checkin_source_seek_incore;
> ++
> ++ source->data.incore.buf = buf;
> ++ source->data.incore.nr_read = 0;
> ++
> ++ source->size = size;
> ++ source->path = path;
> ++}
> ++
> + /*
> + * Read the contents from 'source' for 'size' bytes, streaming it to the
> + * packfile in state while updating the hash in ctx. Signal a failure
> @@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
> return 0;
> }
> @@ bulk-checkin.c: static int deflate_obj_to_pack(struct bulk_checkin_packfile *sta
> + const char *path, enum object_type type,
> + unsigned flags)
> +{
> -+ struct bulk_checkin_source source = {
> -+ .type = SOURCE_INCORE,
> -+ .buf = buf,
> -+ .size = size,
> -+ .read = 0,
> -+ .path = path,
> -+ };
> ++ struct bulk_checkin_source source;
> ++
> ++ init_bulk_checkin_source_incore(&source, buf, size, path);
> +
> + return deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);
> +}
> 6: 60568f9281 = 4: 2670192802 bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
> 7: b9be9df122 ! 5: 3595db76a5 builtin/merge-tree.c: implement support for `--write-pack`
> @@ Documentation/git-merge-tree.txt: OPTIONS
>
> ## builtin/merge-tree.c ##
> @@
> - #include "quote.h"
> #include "tree.h"
> #include "config.h"
> + #include "strvec.h"
> +#include "bulk-checkin.h"
>
> static int line_termination = '\n';
>
> @@ builtin/merge-tree.c: struct merge_tree_options {
> - int show_messages;
> int name_only;
> int use_stdin;
> + struct merge_options merge_options;
> + int write_pack;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
> - init_merge_options(&opt, the_repository);
> + _("not something we can merge"));
>
> opt.show_rename_progress = 0;
> + opt.write_pack = o->write_pack;
> @@ builtin/merge-tree.c: static int real_merge(struct merge_tree_options *o,
> opt.branch1 = branch1;
> opt.branch2 = branch2;
> @@ builtin/merge-tree.c: int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> - &merge_base,
> - N_("commit"),
> N_("specify a merge-base for the merge")),
> + OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> + N_("option for selected merge strategy")),
> + OPT_BOOL(0, "write-pack", &o.write_pack,
> + N_("write new objects to a pack instead of as loose")),
> OPT_END()
>
> base-commit: ceadf0f3cf51550166a387ec8508bb55e7883057
> --
> 2.42.0.425.g963d08ddb3.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Patrick Steinhardt @ 2023-10-25 7:58 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <3595db76a525fcebc3c896e231246704b044310c.1698101088.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 11501 bytes --]
On Mon, Oct 23, 2023 at 06:45:06PM -0400, Taylor Blau wrote:
> When using merge-tree often within a repository[^1], it is possible to
> generate a relatively large number of loose objects, which can result in
> degraded performance, and inode exhaustion in extreme cases.
>
> Building on the functionality introduced in previous commits, the
> bulk-checkin machinery now has support to write arbitrary blob and tree
> objects which are small enough to be held in-core. We can use this to
> write any blob/tree objects generated by ORT into a separate pack
> instead of writing them out individually as loose.
>
> This functionality is gated behind a new `--write-pack` option to
> `merge-tree` that works with the (non-deprecated) `--write-tree` mode.
>
> The implementation is relatively straightforward. There are two spots
> within the ORT mechanism where we call `write_object_file()`, one for
> content differences within blobs, and another to assemble any new trees
> necessary to construct the merge. In each of those locations,
> conditionally replace calls to `write_object_file()` with
> `index_blob_bulk_checkin_incore()` or `index_tree_bulk_checkin_incore()`
> depending on which kind of object we are writing.
>
> The only remaining task is to begin and end the transaction necessary to
> initialize the bulk-checkin machinery, and move any new pack(s) it
> created into the main object store.
>
> [^1]: Such is the case at GitHub, where we run presumptive "test merges"
> on open pull requests to see whether or not we can light up the merge
> button green depending on whether or not the presumptive merge was
> conflicted.
>
> This is done in response to a number of user-initiated events,
> including viewing an open pull request whose last test merge is stale
> with respect to the current base and tip of the pull request. As a
> result, merge-tree can be run very frequently on large, active
> repositories.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> Documentation/git-merge-tree.txt | 4 ++
> builtin/merge-tree.c | 5 ++
> merge-ort.c | 42 +++++++++++----
> merge-recursive.h | 1 +
> t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++
> 5 files changed, 136 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index ffc4fbf7e8..9d37609ef1 100644
> --- a/Documentation/git-merge-tree.txt
> +++ b/Documentation/git-merge-tree.txt
> @@ -69,6 +69,10 @@ OPTIONS
> specify a merge-base for the merge, and specifying multiple bases is
> currently not supported. This option is incompatible with `--stdin`.
>
> +--write-pack::
> + Write any new objects into a separate packfile instead of as
> + individual loose objects.
> +
> [[OUTPUT]]
> OUTPUT
> ------
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index a35e0452d6..218442ac9b 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -19,6 +19,7 @@
> #include "tree.h"
> #include "config.h"
> #include "strvec.h"
> +#include "bulk-checkin.h"
>
> static int line_termination = '\n';
>
> @@ -416,6 +417,7 @@ struct merge_tree_options {
> int name_only;
> int use_stdin;
> struct merge_options merge_options;
> + int write_pack;
> };
>
> static int real_merge(struct merge_tree_options *o,
> @@ -441,6 +443,7 @@ static int real_merge(struct merge_tree_options *o,
> _("not something we can merge"));
>
> opt.show_rename_progress = 0;
> + opt.write_pack = o->write_pack;
>
> opt.branch1 = branch1;
> opt.branch2 = branch2;
> @@ -553,6 +556,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
> N_("specify a merge-base for the merge")),
> OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
> N_("option for selected merge strategy")),
> + OPT_BOOL(0, "write-pack", &o.write_pack,
> + N_("write new objects to a pack instead of as loose")),
> OPT_END()
> };
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 3653725661..523577d71e 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -48,6 +48,7 @@
> #include "tree.h"
> #include "unpack-trees.h"
> #include "xdiff-interface.h"
> +#include "bulk-checkin.h"
>
> /*
> * We have many arrays of size 3. Whenever we have such an array, the
> @@ -2108,10 +2109,19 @@ static int handle_content_merge(struct merge_options *opt,
> if ((merge_status < 0) || !result_buf.ptr)
> ret = error(_("failed to execute internal merge"));
>
> - if (!ret &&
> - write_object_file(result_buf.ptr, result_buf.size,
> - OBJ_BLOB, &result->oid))
> - ret = error(_("unable to add %s to database"), path);
> + if (!ret) {
> + ret = opt->write_pack
> + ? index_blob_bulk_checkin_incore(&result->oid,
> + result_buf.ptr,
> + result_buf.size,
> + path, 1)
> + : write_object_file(result_buf.ptr,
> + result_buf.size,
> + OBJ_BLOB, &result->oid);
> + if (ret)
> + ret = error(_("unable to add %s to database"),
> + path);
> + }
>
> free(result_buf.ptr);
> if (ret)
> @@ -3597,7 +3607,8 @@ static int tree_entry_order(const void *a_, const void *b_)
> b->string, strlen(b->string), bmi->result.mode);
> }
>
> -static int write_tree(struct object_id *result_oid,
> +static int write_tree(struct merge_options *opt,
> + struct object_id *result_oid,
> struct string_list *versions,
> unsigned int offset,
> size_t hash_size)
> @@ -3631,8 +3642,14 @@ static int write_tree(struct object_id *result_oid,
> }
>
> /* Write this object file out, and record in result_oid */
> - if (write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid))
> + ret = opt->write_pack
> + ? index_tree_bulk_checkin_incore(result_oid,
> + buf.buf, buf.len, "", 1)
> + : write_object_file(buf.buf, buf.len, OBJ_TREE, result_oid);
> +
> + if (ret)
> ret = -1;
> +
> strbuf_release(&buf);
> return ret;
> }
> @@ -3797,8 +3814,8 @@ static int write_completed_directory(struct merge_options *opt,
> */
> dir_info->is_null = 0;
> dir_info->result.mode = S_IFDIR;
> - if (write_tree(&dir_info->result.oid, &info->versions, offset,
> - opt->repo->hash_algo->rawsz) < 0)
> + if (write_tree(opt, &dir_info->result.oid, &info->versions,
> + offset, opt->repo->hash_algo->rawsz) < 0)
> ret = -1;
> }
>
> @@ -4332,9 +4349,13 @@ static int process_entries(struct merge_options *opt,
> fflush(stdout);
> BUG("dir_metadata accounting completely off; shouldn't happen");
> }
> - if (write_tree(result_oid, &dir_metadata.versions, 0,
> + if (write_tree(opt, result_oid, &dir_metadata.versions, 0,
> opt->repo->hash_algo->rawsz) < 0)
> ret = -1;
> +
> + if (opt->write_pack)
> + end_odb_transaction();
> +
> cleanup:
> string_list_clear(&plist, 0);
> string_list_clear(&dir_metadata.versions, 0);
> @@ -4878,6 +4899,9 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> */
> strmap_init(&opt->priv->conflicts);
>
> + if (opt->write_pack)
> + begin_odb_transaction();
> +
> trace2_region_leave("merge", "allocate/init", opt->repo);
> }
>
> diff --git a/merge-recursive.h b/merge-recursive.h
> index 3d3b3e3c29..5c5ff380a8 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -48,6 +48,7 @@ struct merge_options {
> unsigned renormalize : 1;
> unsigned record_conflict_msgs_as_headers : 1;
> const char *msg_header_prefix;
> + unsigned write_pack : 1;
>
> /* internal fields used by the implementation */
> struct merge_options_internal *priv;
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index b2c8a43fce..d2a8634523 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -945,4 +945,97 @@ test_expect_success 'check the input format when --stdin is passed' '
> test_cmp expect actual
> '
>
> +packdir=".git/objects/pack"
> +
> +test_expect_success 'merge-tree can pack its result with --write-pack' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> +
> + # base has lines [3, 4, 5]
> + # - side adds to the beginning, resulting in [1, 2, 3, 4, 5]
> + # - other adds to the end, resulting in [3, 4, 5, 6, 7]
> + #
> + # merging the two should result in a new blob object containing
> + # [1, 2, 3, 4, 5, 6, 7], along with a new tree.
> + test_commit -C repo base file "$(test_seq 3 5)" &&
> + git -C repo branch -M main &&
> + git -C repo checkout -b side main &&
> + test_commit -C repo side file "$(test_seq 1 5)" &&
> + git -C repo checkout -b other main &&
> + test_commit -C repo other file "$(test_seq 3 7)" &&
> +
> + find repo/$packdir -type f -name "pack-*.idx" >packs.before &&
> + tree="$(git -C repo merge-tree --write-pack \
> + refs/tags/side refs/tags/other)" &&
> + blob="$(git -C repo rev-parse $tree:file)" &&
> + find repo/$packdir -type f -name "pack-*.idx" >packs.after &&
While we do assert that we write a new packfile, we don't assert whether
parts of the written object may have been written as loose objects. Do
we want to tighten the checks to verify that?
Patrick
> + test_must_be_empty packs.before &&
> + test_line_count = 1 packs.after &&
> +
> + git show-index <$(cat packs.after) >objects &&
> + test_line_count = 2 objects &&
> + grep "^[1-9][0-9]* $tree" objects &&
> + grep "^[1-9][0-9]* $blob" objects
> +'
> +
> +test_expect_success 'merge-tree can write multiple packs with --write-pack' '
> + test_when_finished "rm -rf repo" &&
> + git init repo &&
> + (
> + cd repo &&
> +
> + git config pack.packSizeLimit 512 &&
> +
> + test_seq 512 >f &&
> +
> + # "f" contains roughly ~2,000 bytes.
> + #
> + # Each side ("foo" and "bar") adds a small amount of data at the
> + # beginning and end of "base", respectively.
> + git add f &&
> + test_tick &&
> + git commit -m base &&
> + git branch -M main &&
> +
> + git checkout -b foo main &&
> + {
> + echo foo && cat f
> + } >f.tmp &&
> + mv f.tmp f &&
> + git add f &&
> + test_tick &&
> + git commit -m foo &&
> +
> + git checkout -b bar main &&
> + echo bar >>f &&
> + git add f &&
> + test_tick &&
> + git commit -m bar &&
> +
> + find $packdir -type f -name "pack-*.idx" >packs.before &&
> + # Merging either side should result in a new object which is
> + # larger than 1M, thus the result should be split into two
> + # separate packs.
> + tree="$(git merge-tree --write-pack \
> + refs/heads/foo refs/heads/bar)" &&
> + blob="$(git rev-parse $tree:f)" &&
> + find $packdir -type f -name "pack-*.idx" >packs.after &&
> +
> + test_must_be_empty packs.before &&
> + test_line_count = 2 packs.after &&
> + for idx in $(cat packs.after)
> + do
> + git show-index <$idx || return 1
> + done >objects &&
> +
> + # The resulting set of packs should contain one copy of both
> + # objects, each in a separate pack.
> + test_line_count = 2 objects &&
> + grep "^[1-9][0-9]* $tree" objects &&
> + grep "^[1-9][0-9]* $blob" objects
> +
> + )
> +'
> +
> test_done
> --
> 2.42.0.425.g963d08ddb3.dirty
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Patrick Steinhardt @ 2023-10-25 7:58 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <d8cf8e4395375f88fe4e1ade2b79a3be6ce5fb12.1698101088.git.me@ttaylorr.com>
[-- Attachment #1: Type: text/plain, Size: 6245 bytes --]
On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> Introduce `index_blob_bulk_checkin_incore()` which allows streaming
> arbitrary blob contents from memory into the bulk-checkin pack.
>
> In order to support streaming from a location in memory, we must
> implement a new kind of bulk_checkin_source that does just that. These
> implementation in spread out across:
Nit: the commit message is a bit off here. Probably not worth a reroll
though.
> - init_bulk_checkin_source_incore()
> - bulk_checkin_source_read_incore()
> - bulk_checkin_source_seek_incore()
>
> Note that, unlike file descriptors, which manage their own offset
> internally, we have to keep track of how many bytes we've read out of
> the buffer, and make sure we don't read past the end of the buffer.
>
> This will be useful in a couple of more commits in order to provide the
> `merge-tree` builtin with a mechanism to create a new pack containing
> any objects it created during the merge, instead of storing those
> objects individually as loose.
>
> Similar to the existing `index_blob_bulk_checkin()` function, the
> entrypoint delegates to `deflate_obj_to_pack_incore()`. That function in
> turn delegates to deflate_obj_to_pack(), which is responsible for
> formatting the pack header and then deflating the contents into the
> pack.
>
> Consistent with the rest of the bulk-checkin mechanism, there are no
> direct tests here. In future commits when we expose this new
> functionality via the `merge-tree` builtin, we will test it indirectly
> there.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> bulk-checkin.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++
> bulk-checkin.h | 4 +++
> 2 files changed, 79 insertions(+)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 79776e679e..b728210bc7 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -148,6 +148,10 @@ struct bulk_checkin_source {
> struct {
> int fd;
> } from_fd;
> + struct {
> + const void *buf;
> + size_t nr_read;
> + } incore;
> } data;
>
> size_t size;
> @@ -166,6 +170,36 @@ static off_t bulk_checkin_source_seek_from_fd(struct bulk_checkin_source *source
> return lseek(source->data.from_fd.fd, offset, SEEK_SET);
> }
>
> +static off_t bulk_checkin_source_read_incore(struct bulk_checkin_source *source,
> + void *buf, size_t nr)
> +{
> + const unsigned char *src = source->data.incore.buf;
> +
> + if (source->data.incore.nr_read > source->size)
> + BUG("read beyond bulk-checkin source buffer end "
> + "(%"PRIuMAX" > %"PRIuMAX")",
> + (uintmax_t)source->data.incore.nr_read,
> + (uintmax_t)source->size);
> +
> + if (nr > source->size - source->data.incore.nr_read)
> + nr = source->size - source->data.incore.nr_read;
> +
> + src += source->data.incore.nr_read;
> +
> + memcpy(buf, src, nr);
> + source->data.incore.nr_read += nr;
> + return nr;
> +}
> +
> +static off_t bulk_checkin_source_seek_incore(struct bulk_checkin_source *source,
> + off_t offset)
> +{
> + if (!(0 <= offset && offset < source->size))
> + return (off_t)-1;
At the risk of showing my own ignorance, but why is the cast here
necessary?
Patrick
> + source->data.incore.nr_read = offset;
> + return source->data.incore.nr_read;
> +}
> +
> static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> int fd, size_t size,
> const char *path)
> @@ -181,6 +215,22 @@ static void init_bulk_checkin_source_from_fd(struct bulk_checkin_source *source,
> source->path = path;
> }
>
> +static void init_bulk_checkin_source_incore(struct bulk_checkin_source *source,
> + const void *buf, size_t size,
> + const char *path)
> +{
> + memset(source, 0, sizeof(struct bulk_checkin_source));
> +
> + source->read = bulk_checkin_source_read_incore;
> + source->seek = bulk_checkin_source_seek_incore;
> +
> + source->data.incore.buf = buf;
> + source->data.incore.nr_read = 0;
> +
> + source->size = size;
> + source->path = path;
> +}
> +
> /*
> * Read the contents from 'source' for 'size' bytes, streaming it to the
> * packfile in state while updating the hash in ctx. Signal a failure
> @@ -359,6 +409,19 @@ static int deflate_obj_to_pack(struct bulk_checkin_packfile *state,
> return 0;
> }
>
> +static int deflate_obj_to_pack_incore(struct bulk_checkin_packfile *state,
> + struct object_id *result_oid,
> + const void *buf, size_t size,
> + const char *path, enum object_type type,
> + unsigned flags)
> +{
> + struct bulk_checkin_source source;
> +
> + init_bulk_checkin_source_incore(&source, buf, size, path);
> +
> + return deflate_obj_to_pack(state, result_oid, &source, type, 0, flags);
> +}
> +
> static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
> struct object_id *result_oid,
> int fd, size_t size,
> @@ -421,6 +484,18 @@ int index_blob_bulk_checkin(struct object_id *oid,
> return status;
> }
>
> +int index_blob_bulk_checkin_incore(struct object_id *oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags)
> +{
> + int status = deflate_obj_to_pack_incore(&bulk_checkin_packfile, oid,
> + buf, size, path, OBJ_BLOB,
> + flags);
> + if (!odb_transaction_nesting)
> + flush_bulk_checkin_packfile(&bulk_checkin_packfile);
> + return status;
> +}
> +
> void begin_odb_transaction(void)
> {
> odb_transaction_nesting += 1;
> diff --git a/bulk-checkin.h b/bulk-checkin.h
> index aa7286a7b3..1b91daeaee 100644
> --- a/bulk-checkin.h
> +++ b/bulk-checkin.h
> @@ -13,6 +13,10 @@ int index_blob_bulk_checkin(struct object_id *oid,
> int fd, size_t size,
> const char *path, unsigned flags);
>
> +int index_blob_bulk_checkin_incore(struct object_id *oid,
> + const void *buf, size_t size,
> + const char *path, unsigned flags);
> +
> /*
> * Tell the object database to optimize for adding
> * multiple objects. end_odb_transaction must be called
> --
> 2.42.0.425.g963d08ddb3.dirty
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Uwe Kleine-König @ 2023-10-25 7:43 UTC (permalink / raw)
To: Michael Strawbridge
Cc: Jeff King, Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com>
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
Hello,
On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> >From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <michael.strawbridge@amd.com>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH] send-email: move validation code below process_address_list
>
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
>
> This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
>
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
If you do Fixes: trailers as the kernel does, this could get:
Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
I tested this patch on top of main (2e8e77cbac8a) and it fixes the
regression I reported in a separate thread (where Jeff pointed out this
patch as fixing it).
Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Thanks
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
* Re: [PATCH v5 1/5] bulk-checkin: extract abstract `bulk_checkin_source`
From: Jeff King @ 2023-10-25 7:37 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Elijah Newren, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <696aa027e46ddec310812fad2d4b12082447d925.1698101088.git.me@ttaylorr.com>
On Mon, Oct 23, 2023 at 06:44:56PM -0400, Taylor Blau wrote:
> +struct bulk_checkin_source {
> + off_t (*read)(struct bulk_checkin_source *, void *, size_t);
> + off_t (*seek)(struct bulk_checkin_source *, off_t);
> +
> + union {
> + struct {
> + int fd;
> + } from_fd;
> + } data;
> +
> + size_t size;
> + const char *path;
> +};
The virtual functions combined with the union are a funny mix of
object-oriented and procedural code. The bulk_checkin_source has
totally virtualized functions, but knows about all of the ancillary data
each set of virtualized functions might want. ;)
I think the more pure OO version would embed the parent, and have each
concrete type define its own struct type, like:
struct bulk_checkin_source_fd {
struct bulk_checkin_source src;
int fd;
};
That works great if the code which constructs it knows which concrete
type it wants, and can just do:
struct bulk_checkin_source_fd src;
init_bulk_checkin_source_from_fd(&src, ...);
If even the construction is somewhat virtualized, then you are stuck
with heap constructors like:
struct bulk_checkin_source *bulk_checkin_source_from_fd(...);
Not too bad, but you have to remember to free now.
Alternatively, I think some of our other OO code just leaves room for
a type-specific void pointer, like:
struct bulk_checkin_source {
...the usual stuff...
void *magic_type_data;
};
and then the init_bulk_checkin_source_from_fd() function allocates its
own heap struct for the magic_type_data field and sticks the int in
there.
That said, both of those are a lot more annoying to use in C (more
boilerplate, more casting, and more opportunities to get something
wrong, including leaks). So I don't mind this in-between state. It is a
funny layering violating from an OO standpoint, but it's not like we
expect an unbounded set of concrete types to "inherit" from the source
struct.
-Peff
^ permalink raw reply
* 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-25 7:40 UTC (permalink / raw)
To: Jeff King; +Cc: Luben Tuikov, Michael Strawbridge, git, entwicklung
In-Reply-To: <20231025072104.GA2145145@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]
Hello,
On Wed, Oct 25, 2023 at 03:21:04AM -0400, Jeff King wrote:
> On Tue, Oct 24, 2023 at 10:43:18PM +0200, Uwe Kleine-König wrote:
>
> > I debugged a bit and if I do
> >
> > mv .git/hooks/sendemail-validate .git/hooks/sendemail-validate.bak
> >
> > git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'
> > starts to work for me, too.
> >
> > I'd guess the content of my sendemail-validate script doesn't matter
> > much, but for the record, it's:
> >
> > #!/bin/sh
> > # installed by patatt install-hook
> > patatt sign --hook "${1}"
> >
> > Does the problem reproduce on your end with a sendemail-validate script?
>
> I can reproduce with:
>
> git init
> echo foo >file && git add file && git commit -m foo
> echo 'exit 0' >.git/hooks/sendemail-validate
> chmod +x .git/hooks/sendemail-validate
> git send-email --dry-run --to='pëff <peff@peff.net>' -1
>
> Note that the bug will only trigger if Email::Valid is installed.
I can confirm I have this package installed (via Debian's
libemail-valid-perl).
> I think this is the same issue being discussed elsewhere. The call to
> process_address_list() sanitizes it to use rfc2047 encoding, which is
> necessary for it to be syntactically valid.
>
> So the patch to move the validation later in the process here:
>
> https://lore.kernel.org/git/ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com/
>
> fixes it.
Tested and indeed that fixes my usecase. With that patch also the
original regression is fixed and I can do
git send-email -1 --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>, pëff <peff@peff.net>'
again. \o/
Thanks
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
* Re: [PATCH v1 3/4] config: factor out global config file retrievalync-mailbox>
From: Kristoffer Haugsbakk @ 2023-10-25 7:33 UTC (permalink / raw)
To: Patrick Steinhardt, Taylor Blau; +Cc: git, stolee
In-Reply-To: <ZTip7JWm-WRWTImU@tanuki>
On Wed, Oct 25, 2023, at 07:38, Patrick Steinhardt wrote:
>> What do you guys think the signature of `git_global_config` should be?
>
> Either of the following:
>
> - `int git_global_config(char **out_pat)`
> - `char **git_global_config(void)`
>
> In the first case you'd signal error via a non-zero return value,
> whereas in the second case you would signal it via a `NULL` return
> value.
>
> To decide which one to go with I'd recommend to check whether there is
> any similar precedent in "config.h" and what style that precedent uses.
Okay thanks. So no parameter for determining whether one is writing or
just reading the file.
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"
From: Jeff King @ 2023-10-25 7:21 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Michael Strawbridge, Luben Tuikov, git, entwicklung
In-Reply-To: <20231024204318.gi6b4ygqbilm2yke@pengutronix.de>
On Tue, Oct 24, 2023 at 10:43:18PM +0200, Uwe Kleine-König wrote:
> I debugged a bit and if I do
>
> mv .git/hooks/sendemail-validate .git/hooks/sendemail-validate.bak
>
> git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'
> starts to work for me, too.
>
> I'd guess the content of my sendemail-validate script doesn't matter
> much, but for the record, it's:
>
> #!/bin/sh
> # installed by patatt install-hook
> patatt sign --hook "${1}"
>
> Does the problem reproduce on your end with a sendemail-validate script?
I can reproduce with:
git init
echo foo >file && git add file && git commit -m foo
echo 'exit 0' >.git/hooks/sendemail-validate
chmod +x .git/hooks/sendemail-validate
git send-email --dry-run --to='pëff <peff@peff.net>' -1
Note that the bug will only trigger if Email::Valid is installed. I
think this is the same issue being discussed elsewhere. The call to
process_address_list() sanitizes it to use rfc2047 encoding, which is
necessary for it to be syntactically valid.
So the patch to move the validation later in the process here:
https://lore.kernel.org/git/ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com/
fixes it.
-Peff
^ permalink raw reply
* Re: [PATCH] send-email: move validation code below process_address_list
From: Jeff King @ 2023-10-25 6:50 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
In-Reply-To: <ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com>
On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
>
> This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
Is there a test we can include here?
> @@ -2023,6 +1999,30 @@ sub process_file {
> return 1;
> }
>
> +if ($validate) {
So the new spot is right before we call process_file() on each of the
input files. It is a little hard to follow because of the number of
functions defined inline in the middle of the script, but I think that
is a reasonable spot. It is after we have called process_address_list()
on to/cc/bcc, which I think fixes the regression. But it is also after
we sanitize $reply_to, etc, which seems like a good idea.
But I think putting it down that far is the source of the test failures.
The culprit seems not to be the call to validate_patch() in the loop you
moved, but rather pre_process_file(), which was added in your earlier
a8022c5f7b (send-email: expose header information to git-send-email's
sendemail-validate hook, 2023-04-19).
It looks like the issue is the global $message_num variable which is
incremented by pre_process_file(). On line 1755 (on the current tip of
master), we set it to 0. And your patch moves the validation across
there (from line ~799 to ~2023).
And that's why the extra increments didn't matter when you added the
calls to pre_process_file() in your earlier patch; they all happened
before we reset $message_num to 0. But now they happen after.
To be fair, this is absolutely horrific perl code. There's over a
thousand lines of function definitions, and then hidden in the middle
are some global variable assignments!
So we have a few options, I think:
1. Reset $message_num to 0 after validating (maybe we also need
to reset $in_reply_to, etc, set at the same time? I didn't check).
This feels like a hack.
2. Move the validation down, but not so far down. Like maybe right
after we set up the @files list with the $compose.final name. This
is the smallest diff, but feels like we haven't really made the
world a better place.
3. Move the $message_num, etc, initialization to right before we call
the process_file() loop, which is what expects to use them. Like
this (squashed into your patch):
diff --git a/git-send-email.perl b/git-send-email.perl
index a898dbc76e..d44db14223 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1730,10 +1730,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) = @_;
@@ -2023,6 +2019,10 @@ sub process_file {
delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
}
+$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
That seems to make the test failures go away. It is still weird that the
validation code is calling pre_process_file(), which increments
$message_num, without us having set it up in any meaningful way. I'm not
sure if there are bugs lurking there or not. I'm not impressed by the
general quality of this code, and I'm kind of afraid to keep looking
deeper.
-Peff
^ 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