* Re: [PATCH v6] merge-tree: add -X strategy option
From: Phillip Wood @ 2023-10-09 9:58 UTC (permalink / raw)
To: Izzy via GitGitGadget, git; +Cc: Elijah Newren, Jeff King, Izzy
In-Reply-To: <pull.1565.v6.git.1695522222723.gitgitgadget@gmail.com>
On 24/09/2023 03:23, Izzy via GitGitGadget wrote:
Thanks for updating the patch, sorry it has taken me a while to look at
this.
> From: Tang Yuyi <winglovet@gmail.com>
>
> Add merge strategy option to produce more customizable merge result such
> as automatically resolving conflicts.
>
> Signed-off-by: Tang Yuyi <winglovet@gmail.com>
> ---
> static int real_merge(struct merge_tree_options *o,
> @@ -423,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
> {
> struct commit *parent1, *parent2;
> struct commit_list *merge_bases = NULL;
> - struct merge_options opt;
> + struct merge_options opt = o->merge_options;
Copying struct merge_options by value here is unusual in our code base.
I don't think it introduces a bug (there is no function to free the
resources allocated in struct merge_options so we do not end up freeing
them twice for example) but it would be clearer that it was safe if you did
struct merge_options *opt = &o->merge_options;
and updated the code to reflect that opt is now a pointer or just
replaced all uses of "opt" with "o->merge_options"
> + if (xopts.nr && o.mode == MODE_TRIVIAL)
> + die(_("--trivial-merge is incompatible with all other options"));
This is good, thanks for adding it.
> + for (int x = 0; x < xopts.nr; x++)
This is not worth a re-roll on its own but as xopts.nr is a size_t we
should declare x to be the same type rather than an int.
The tests are pretty minimal, ideally we would check that "-X" works
with "--stdin", that it is rejected by a trivial merge and that one of
the other strategy options works.
Best Wishes
Phillip
^ permalink raw reply
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Luma @ 2023-10-09 9:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <CAFR+8DyN8vbuvdgZPkSVqS2=sqconwhx3QfcpJ0+Wi_oCA=s0w@mail.gmail.com>
Dear Git Community and Mentors,
I hope this email finds you well. My name is Achu Luma, and I am
excited to submit my application for the Outreachy program with the
Git project.
I have been a passionate open-source enthusiast and a dedicated Git
user for two years, and I am thrilled at the opportunity to contribute
to the Git community.
Introduction:
----------------
I study Computer Science from the University of Bamenda. Over the past
4 years, I have gained experience in software development and have
participated in various class projects.
Why I am a Good Fit:
----------------------
1. Proficient with Git: I have a good understanding of Git's version
control system and have successfully used it in both personal and
educational projects.
2. Strong Programming Skills: My programming skills in python, C etc
and experience with git, shell etc make me well-prepared to contribute
to Git's codebase.
3. Open Source Involvement: I have actively contributed to git
open-source project, including
https://public-inbox.org/git/20231003174853.1732-1-ach.lumap@gmail.com/T/#t
, where I have submitted a patch that has been well-received.
Project Idea - Moving Existing Tests to a Unit Testing Framework:
------------------------------------------------------------------
I am excited about "Moving Existing Tests to a Unit Testing Framework".
The objective of this project is to enhance the efficiency and
maintainability of Git's testing infrastructure by porting existing
unit tests to a unit testing framework.
**Project Plan**:
- Evaluate the existing tests in the `t/helper/` directory to identify
those suitable for migration to the unit testing framework.
- Develop a migration strategy and create detailed plans for adapting
these tests.
- Port the identified tests to the unit testing framework while
ensuring they maintain their functionality.
- Verify the correctness and reliability of the migrated tests through
thorough testing and validation.
- Collaborate with the Git community to gather feedback and make
necessary adjustments.
**Timeline**:
- Community Bonding (Oct 2 - Nov 20): Familiarize myself with the Git
project and establish communication channels.
- Coding Phase (Dec 4 - Jan 15): Implement the migration of tests and
seek feedback from mentors and the community.
- Testing and Validation (Jan 15 - Feb 15): Rigorously test the
migrated tests and make improvements based on feedback.
- Documentation and Finalization (Feb 15 - March 1): Document the
migration process and finalize the project.
**Contribution to Git Community**:
I have actively participated in Git's mailing-list discussions and
submitted a patch(
https://public-inbox.org/git/20231003174853.1732-1-ach.lumap@gmail.com/T/#t)
for review. I have received positive feedback on my contributions, and
it has been queued to be merged into official Git branches maintained
by Junio. Additionally, I have been involved in discussions related to
the git project.(https://public-inbox.org/git/CAFR+8DyN8vbuvdgZPkSVqS2=sqconwhx3QfcpJ0+Wi_oCA=s0w@mail.gmail.com/T/#t)
**Proposal Drafts**:
I have shared drafts of this proposal on the Git mailing list
git@vger.kernel.org and will incorporate valuable feedback provided
by the community.
**Next Steps**:
I am eager to discuss my proposal further and collaborate with the Git
community to ensure the success of this project. I will continue to
engage with the community, seek guidance, and refine my proposal as
per your suggestions.
Thank you for considering my application. I look forward to the
opportunity to contribute to the Git project and help make it even
more robust and reliable.
Best Regards,
On Wed, Oct 4, 2023 at 12:36 AM Luma <ach.lumap@gmail.com> wrote:
>
> oh yes, "Move existing tests to a unit testing framework" was the
> only listed project for this current Outreachy cohort. So, I used it
> to express my intent.
> I appreciate the clarification on authorship identity for patches. I
> will update subsequent patches with a legal full name to conform to
> the community rules.
>
> Regards.
>
> On Tue, Oct 3, 2023 at 7:51 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Luma <ach.lumap@gmail.com> writes:
> >
> > > Hi;
> > > My name is Luma, and I wanted to take a moment to introduce myself
> > > and share some
> > > insights on an essential aspect of avoiding pipes in git related
> > > commands in test scripts.
> > >
> > > I am an outreachy applicant for the December 2023 cohort and look
> > > forward to learning from you.
> >
> > I notice that the title of the message and the immediate topic you
> > discuss in the body of the message do not match. I presume that the
> > topic on the title is what you prefer to work on if the unit testing
> > framework is ready by the time Outreachy program starts, and the
> > mention about "do not clobber exit code of Git with pipes in the
> > tests" is your "dip the tip of a toe in water" microproject?
> >
> > Welcome to the Git development community.
> >
> > Do you have a single word name? If so please disregard the below,
> > but in case "Luma" is just a nickname (e.g. like I am introducing
> > myself to my Git friends "Hi, I am Gitster!") you use online, please
> > read on.
> >
> > For signing off your patches, we'd prefer to see your real name
> > used, as Signed-off-by: is meant to have legal significance. And
> > because we also expect the authorship identity to match the
> > name/e-mail of the sign-off, it would mean your patch submissions
> > are expected to look like:
> >
> > From: Luma <ach.lumap@gmail.com>
> > Subject: ... title of the patch goes here ...
> >
> > ... body of the proposed commit log message goes here...
> >
> > Signed-off-by: Luma <ach.lumap@gmail.com>
> >
> > but "Luma" replaced with your full real name.
> >
> > Thanks.
^ permalink raw reply
* Re: [PATCH] doc/cat-file: clarify description regarding various command forms
From: Štěpán Němec @ 2023-10-09 8:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, avarab
In-Reply-To: <20231005210125.GA981206@coredump.intra.peff.net>
On Thu, 5 Oct 2023 17:01:25 -0400
Jeff King wrote:
> On Thu, Oct 05, 2023 at 07:48:52PM +0200, Štěpán Němec wrote:
>
>> > Yeah, I think that is a big improvement over the status quo. I might
>> > also be worth starting with a single-sentence overview of what is common
>> > to both modes. Something like:
>> >
>> > Output the contents or details of one or more objects. [...]
>>
>> I thought about that when proposing the rewrite, but feel that it would
>> again just duplicate what's said elsewhere, in this case even before,
>> not after, in the very first line of the man page:
>>
>> git-cat-file - Provide content or type and size information for
>> repository objects
>
> Ah, true, I was thinking that the DESCRIPTION section would be the first
> thing users would read, but I didn't notice the headline. I agree that
> what it says is probably sufficient (though arguably "type and size" is
> slightly inaccurate there; I said "details" in my proposed text but
> maybe that is too vague).
We could also leave the NAME vague(r) and put an additional sentence at
the beginning of DESCRIPTION:
NAME
git-cat-file - Provide contents or details of repository objects
SYNOPSIS
[...]
DESCRIPTION
Output the contents or other properties such as size, type or delta
information of one or more objects.
The command can operate [...]
--
Štěpán
^ permalink raw reply
* Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Christian Couder @ 2023-10-09 8:22 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2zcU+o6av=CQy7WAG7DZmNEERcPqB_W3Cmub4w25V3K4g@mail.gmail.com>
On Mon, Oct 9, 2023 at 9:57 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> Thank you very much for the feedback Christian, I'd definitely keep
> this in mind the next time I'm creating/sending a patch.
Great!
> You also said asides working on my commit message, the patch looks
> good,
Yeah, but first others might find some issues in the patch itself,
also, as I say elsewhere, I think it's worth it for you to send a
version 2 of the patch that fixes the commit message issues I pointed
out.
> but as I'm applying to Outreachy, I need to send in my proof of
> contribution as a link of a task I worked on.
> Would sending a link of the mailing list , with the url of this patch
> I just worked on suffice?Something like this :
> https://public-inbox.org/git/20231009011652.1791-1-naomi.ibeh69@gmail.com/T/#u
> I ask because since I used patches to send to Git , I have no pull
> request link to submit.
Yeah, links to the emails you sent in a mailing list archive (Public
Inbox or lore.kernel.org/git) are fine.
^ permalink raw reply
* Re: [PATCH 0/1] *** EDITED add.c ***
From: Naomi Ibe @ 2023-10-09 8:14 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD3Cj7nC704Rti8jBp6VxBJDxsBpf_8vvordW8MWXc_GDA@mail.gmail.com>
Okay then, I'd wait for additional feedback and send in a Version 2
later. Thank you
On Mon, Oct 9, 2023 at 9:03 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Mon, Oct 9, 2023 at 9:46 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
> >
> > Okay I understand it better now ,can I do anything about it? Or should
> > I leave it as is?.
>
> I think you should send a version 2 of the patch (so without a cover
> letter) taking into account the comments I made about it. You might
> want to wait a bit before sending version 2 though, so that others
> might have time to read your patch and give you more feedback about
> it.
>
> > It's my first time using the "git patch
>
> I guess you mean `git format-patch`.
>
> > and "git send-email"
> > commands, so it was a real struggle understanding the proper way to
> > get things setup.
>
> Yeah, it's not easy to set things up, but once that's done it should
> be much easier. To make things even easier, you could perhaps save
> somewhere the commands that you have previously used. And to improve
> or make changes to those commands, you could take a look at the
> documentation for `git format-patch` and `git send-email` which have a
> number of interesting options, and take note of those interesting
> options.
^ permalink raw reply
* Re: [PATCH 0/1] *** EDITED add.c ***
From: Christian Couder @ 2023-10-09 8:03 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2xfUYVmvhZ_r447uArW-_6yK0r7V9BkzX7i+E7MvruA9Q@mail.gmail.com>
On Mon, Oct 9, 2023 at 9:46 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> Okay I understand it better now ,can I do anything about it? Or should
> I leave it as is?.
I think you should send a version 2 of the patch (so without a cover
letter) taking into account the comments I made about it. You might
want to wait a bit before sending version 2 though, so that others
might have time to read your patch and give you more feedback about
it.
> It's my first time using the "git patch
I guess you mean `git format-patch`.
> and "git send-email"
> commands, so it was a real struggle understanding the proper way to
> get things setup.
Yeah, it's not easy to set things up, but once that's done it should
be much easier. To make things even easier, you could perhaps save
somewhere the commands that you have previously used. And to improve
or make changes to those commands, you could take a look at the
documentation for `git format-patch` and `git send-email` which have a
number of interesting options, and take note of those interesting
options.
^ permalink raw reply
* Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Naomi Ibe @ 2023-10-09 7:57 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD35DBBwQ1Mgc+NGVoh1ReLncAz9OJF3Yj++FFrESw8rtw@mail.gmail.com>
Thank you very much for the feedback Christian, I'd definitely keep
this in mind the next time I'm creating/sending a patch.
You also said asides working on my commit message, the patch looks
good, but as I'm applying to Outreachy, I need to send in my proof of
contribution as a link of a task I worked on.
Would sending a link of the mailing list , with the url of this patch
I just worked on suffice?Something like this :
https://public-inbox.org/git/20231009011652.1791-1-naomi.ibeh69@gmail.com/T/#u
I ask because since I used patches to send to Git , I have no pull
request link to submit.
On Mon, Oct 9, 2023 at 8:28 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Mon, Oct 9, 2023 at 3:17 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
> >
> > From: Naomi <naomi.ibeh69@gmail.com>
>
> First the subject should start, after "[PATCH 1/1][Outreachy]", with
> the area of the code you are changing followed by ":", so here "add:"
> (no need for ".c").
>
> Also even if the subject gives a lot of information already, it's
> better to use the body of the commit message to give a bit more
> context and details. For example here either the subject or the body
> of the commit message should say which specific guideline(s) the patch
> is enforcing.
>
> > Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> > ---
> > builtin/add.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
>
> Otherwise the patch looks good to me.
>
> Thanks!
^ permalink raw reply
* Re: [PATCH 0/1] *** EDITED add.c ***
From: Naomi Ibe @ 2023-10-09 7:45 UTC (permalink / raw)
To: Christian Couder; +Cc: git
In-Reply-To: <CAP8UFD3HZOMJTxW5EkUwKu48GebSKX3-EPD8tjGEQnE2MGaZ7w@mail.gmail.com>
Okay I understand it better now ,can I do anything about it? Or should
I leave it as is?.
It's my first time using the "git patch" and "git send-email"
commands, so it was a real struggle understanding the proper way to
get things setup.
On Mon, Oct 9, 2023 at 8:39 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Mon, Oct 9, 2023 at 3:19 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
> >
> > *** BLURB HERE ***
>
> A patch with number 0/X (where X is any number) is called a "cover
> letter" and is supposed to explain the purpose of the patch series it
> is part of. Usually a cover letter is not useful for a single patch
> long patch series (when X is 1), as the commit message of the patch
> should be enough.
>
> Also things like "*** BLURB HERE ***" (including the stars) are
> supposed to be replaced by a subject or some comments to explain
> things.
^ permalink raw reply
* Re: [PATCH 0/1] *** EDITED add.c ***
From: Christian Couder @ 2023-10-09 7:38 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <20231009011842.1956-1-naomi.ibeh69@gmail.com>
On Mon, Oct 9, 2023 at 3:19 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> *** BLURB HERE ***
A patch with number 0/X (where X is any number) is called a "cover
letter" and is supposed to explain the purpose of the patch series it
is part of. Usually a cover letter is not useful for a single patch
long patch series (when X is 1), as the commit message of the patch
should be enough.
Also things like "*** BLURB HERE ***" (including the stars) are
supposed to be replaced by a subject or some comments to explain
things.
^ permalink raw reply
* Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Christian Couder @ 2023-10-09 7:27 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <20231009011652.1791-1-naomi.ibeh69@gmail.com>
On Mon, Oct 9, 2023 at 3:17 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> From: Naomi <naomi.ibeh69@gmail.com>
First the subject should start, after "[PATCH 1/1][Outreachy]", with
the area of the code you are changing followed by ":", so here "add:"
(no need for ".c").
Also even if the subject gives a lot of information already, it's
better to use the body of the commit message to give a bit more
context and details. For example here either the subject or the body
of the commit message should say which specific guideline(s) the patch
is enforcing.
> Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> ---
> builtin/add.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
Otherwise the patch looks good to me.
Thanks!
^ permalink raw reply
* Re: [PATCH 00/25] Documentation fixes
From: Taylor Blau @ 2023-10-09 1:44 UTC (permalink / raw)
To: Elijah Newren via GitGitGadget; +Cc: git, Elijah Newren
In-Reply-To: <pull.1595.git.1696747527.gitgitgadget@gmail.com>
On Sun, Oct 08, 2023 at 06:45:02AM +0000, Elijah Newren via GitGitGadget wrote:
> It turns out that AI is pretty good at making small fixes to documentation;
> certainly not perfect, but it provides quite good signal. Unfortunately,
> there is a lot to sift through. Some points about my strategy:
Quite interesting ;-).
I'm curious to learn a little bit more about your
strategy beyond what you wrote:
- What tool did you use? ChatGPT? Something home-grown?
- (Assuming this was generated by some sort of LLM): what did you
prompt it with?
- What was the output format: the edited text in its entirety, or a
patch that can be applied on top?
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-09 1:37 UTC (permalink / raw)
To: Jeff King; +Cc: Elijah Newren, Junio C Hamano, git, Eric W. Biederman
In-Reply-To: <20231008173329.GA1557002@coredump.intra.peff.net>
On Sun, Oct 08, 2023 at 01:33:29PM -0400, Jeff King wrote:
> On Sun, Oct 08, 2023 at 12:04:04PM -0400, Taylor Blau wrote:
>
> > > I was interested in the same question as Junio, but from a different
> > > angle. fast-import documentation points out that the packs it creates
> > > are suboptimal with poorer delta choices. Are the packs created by
> > > bulk-checkin prone to the same issues? When I was thinking in terms
> > > of having "git merge" use fast-import for pack creation instead of
> > > writing loose objects (an idea I never investigated very far), I was
> > > wondering if I'd need to mark those packs as "less optimal" and do
> > > something to make sure they were more likely to be repacked.
> > >
> > > I believe geometric repacking didn't exist back when I was thinking
> > > about this, and perhaps geometric repacking automatically handles
> > > things nicely for us. Does it, or are we risking retaining
> > > sub-optimal deltas from the bulk-checkin code?
> > >
> > > (I've never really cracked open the pack code, so I have absolutely no
> > > idea; I'm just curious.)
> >
> > Yes, the bulk-checkin mechanism suffers from an even worse problem which
> > is the pack it creates will contain no deltas whatsoever. The contents
> > of the pack are just getting written as-is, so there's no fancy
> > delta-ficiation going on.
>
> I wonder how big a deal this would be in practice for merges.
> pack-objects will look for deltas between any two candidates objects,
> but in practice I think most deltas are between objects from multiple
> commits (across the "time" dimension, if you will) rather than within a
> single tree (the "space" dimension). And a merge operation is generally
> creating a single new tree (recursive merging may create intermediate
> states which would delta, but we don't actually need to keep those
> intermediate ones. I won't be surprised if we do, though).
>
> We should be able to test that theory by looking at existing deltas.
> Here's a script which builds an index of blobs and trees to the commits
> that introduce them:
>
> git rev-list HEAD |
> git diff-tree --stdin -r -m -t --raw |
> perl -lne '
> if (/^[0-9a-f]/) {
> $commit = $_;
> } elsif (/^:\S+ \S+ \S+ (\S+)/) {
> $h{$1} = $commit;
> }
> END { print "$_ $h{$_}" for keys(%h) }
> ' >commits.db
>
> And then we can see which deltas come from the same commit:
>
> git cat-file --batch-all-objects --batch-check='%(objectname) %(deltabase)' |
> perl -alne '
> BEGIN {
> open(my $fh, "<", "commits.db");
> %commit = map { chomp; split } <$fh>;
> }
> next if $F[1] =~ /0{40}/; # not a delta
> next unless defined $commit{$F[0]}; # not in index
> print $commit{$F[0]} eq $commit{$F[1]} ? "inner" : "outer", " ", $_;
> '
>
> In git.git, I see 460 "inner" deltas, and 193,081 "outer" ones. The
> inner ones are mostly small single-file test vectors, which makes sense.
> It's possible to have a merge result that does conflict resolution in
> two such files (that would then delta), but it seems like a fairly
> unlikely case. Numbers for linux.git are similar.
>
> So it might just not be a big issue at all for this use case.
Very interesting, thanks for running (and documenting!) this experiment.
I'm mostly with you that it probably doesn't make a huge difference in
practice here.
One thing that I'm not entirely clear on is how we'd treat objects that
could be good delta candidates for each other between two packs. For
instance, if I write a tree corresponding to the merge between two
branches, it's likely that the resulting tree would be a good delta
candidate against either of the trees at the tips of those two refs.
But we won't pack those trees (the ones at the tips of the refs) in the
same pack as the tree containing their merge. If we later on tried to
repack, would we evaluate the tip trees as possible delta candidates
against the merged tree? Or would we look at the merged tree, realize it
isn't delta'd with anything, and then not attempt to find any
candidates?
> > I think Michael Haggerty (?) suggested to me off-list that it might be
> > interesting to have a flag that we could mark packs with bad/no deltas
> > as such so that we don't implicitly trust their contents as having high
> > quality deltas.
>
> I was going to suggest the same thing. ;) Unfortunately it's a bit
> tricky to do as we have no room in the file format for an optional flag.
> You'd have to add a ".mediocre-delta" file or something.
Yeah, I figured that we'd add a new ".baddeltas" file or something. (As
an aside, we probably should have an optional flags section in the .pack
format, since we seem to have a lot of optional pack extensions: .rev,
.bitmap, .keep, .promisor, etc.)
> But here's another approach. I recall discussing a while back the idea
> that we should not necessarily trust the quality of deltas in packs that
> are pushed (and I think Thomas Gummerer even did some experiments inside
> GitHub with those, though I don't remember the results). And one way
> around that is during geometric repacking to consider the biggest/oldest
> pack as "preferred", reuse its deltas, but always compute from scratch
> with the others (neither reusing on-disk deltas, nor skipping
> try_delta() when two objects come from the same pack).
>
> That same strategy would work here (and for incremental fast-import
> packs, though of course not if your fast-import pack is the "big" one
> after you do a from-scratch import).
Yeah, definitely. I think that that code is still living in GitHub's
fork, but inactive since we haven't set any of the relevant
configuration in GitHub's production environment.
> Possibly it exacerbates the "no deltas" issue from above (though it
> would depend on the command). The bigger question to me is one of
> checkpointing. When do we finish off the pack with a .idx and make it
> available to other readers? We could do it at program exit, but I
> suspect there are some commands that really want to make objects
> available sooner (e.g., as soon as "git add" writes an index, we'd want
> those objects to already be available). Probably every program that
> writes objects would need to be annotated with a checkpoint call (which
> would be a noop in loose object mode).
>
> So maybe it's a dumb direction. I dunno.
I wouldn't say it's a dumb direction ;-). But I'd be hesitant pursuing
it without solving the "no deltas" question from earlier.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 6/7] bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-09 1:31 UTC (permalink / raw)
To: Eric Biederman; +Cc: git, Elijah Newren, Jeff King, Junio C Hamano
In-Reply-To: <E81727B0-A523-4A45-A606-61442357291D@gmail.com>
On Fri, Oct 06, 2023 at 10:07:20PM -0500, Eric Biederman wrote:
> On October 6, 2023 5:02:04 PM CDT, Taylor Blau <me@ttaylorr.com> wrote:
> >Within `deflate_tree_to_pack_incore()`, the changes should be limited
> >to something like:
> >
> > if (the_repository->compat_hash_algo) {
> > struct strbuf converted = STRBUF_INIT;
> > if (convert_object_file(&compat_obj,
> > the_repository->hash_algo,
> > the_repository->compat_hash_algo, ...) < 0)
> > die(...);
> >
> > format_object_header_hash(the_repository->compat_hash_algo,
> > OBJ_TREE, size);
> >
> > strbuf_release(&converted);
> > }
> >
> >, assuming related changes throughout the rest of the bulk-checkin
> >machinery necessary to update the hash of the converted object, which
> >are likewise minimal in size.
>
> So this is close. Just in case someone wants to
> go down this path I want to point out that
> the converted object need to have the compat hash computed over it.
>
> Which means that the strbuf_release in your example comes a bit early.
Doh. You're absolutely right. Let's fix this if/when we cross that
bridge ;-).
Thanks,
Taylor
^ permalink raw reply
* Re: [OUTREACHY] git send-email issues
From: Naomi Ibe @ 2023-10-09 1:25 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <CACS=G2wnQEVqiUA8Jy=iaaAnRv7bMKmegR2rGR=Tbhf7UuLR4w@mail.gmail.com>
I finally got the hang of it!! Apparently I was supposed to set
encryption to "ssl" and serverport to 465, but for the past two days
I'd been using "tls" for encryption and serverport as 587, once I
exchanged them, all was done.
I wasn't able to get the hang of gitgitgadget tonight but I'd
definitely be looking into it in the coming days.
On Sun, Oct 8, 2023 at 11:37 PM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> Oh wow
> I read about gitgadget but didn't understand how it works, I'd
> definitely check it out with these links you sent and I really hope it
> works. Thank you very very much
>
> On Sun, Oct 8, 2023 at 11:17 PM Kristoffer Haugsbakk
> <code@khaugsbakk.name> wrote:
> >
> > Hi Naomi
> >
> > On Sun, Oct 8, 2023, at 23:59, Naomi Ibe wrote:
> > > I've used the --smtp-debug tag, checked the official docs and other
> > > docs too, plus stackoverflow,google and even chatgpt, but nothing
> > > seems to be working. I've even had to change my gmail password tonight
> > > but it still doesn't work. Any tips at all would be greatly
> > > appreciated at this point. Thank you
> >
> > Here [1] is a good resource for setting up Gmail for git-send-email. It's
> > a bit of a chore to set up but that resource was enough for me to get it
> > working. One of the things that you are going to need is an App password.
> >
> > But have you considered using GitGitGadget instead?[2] You can make a pull
> > request on that repository and then the program (gitgitgadget) can send
> > the emails to this mailing list for you.
> >
> > [1] https://stackoverflow.com/a/68238913/1725151
> >
> > [2] https://github.com/gitgitgadget/gitgitgadget/
> >
> > --
> > Kristoffer
^ permalink raw reply
* Re: [PATCH] repack: free existing_cruft array after use
From: Taylor Blau @ 2023-10-09 1:24 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Patrick Steinhardt, Eric Sunshine
In-Reply-To: <20231007172031.GA1524950@coredump.intra.peff.net>
On Sat, Oct 07, 2023 at 01:20:31PM -0400, Jeff King wrote:
> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:
>
> > +static void collapse_small_cruft_packs(FILE *in, size_t max_size,
> > + struct existing_packs *existing)
> > +{
> > + struct packed_git **existing_cruft, *p;
> > + struct strbuf buf = STRBUF_INIT;
> > [...]
> > +
> > + strbuf_release(&buf);
> > +}
>
> Coverity (using the just-merged-to-next version of the workflow file!)
> flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in
> 'next', I think we'd want this on top:
Woohoo! I'm glad that this is already paying dividends.
> -- >8 --
> Subject: [PATCH] repack: free existing_cruft array after use
>
> We allocate an array of packed_git pointers so that we can sort the list
> of cruft packs, but we never free the array, causing a small leak. Note
> that we don't need to free the packed_git structs themselves; they're
> owned by the repository object.
Thanks, I can't believe I missed this when writing this function. The
fix looks obviously correct to me, so this has my:
Acked-by: Taylor Blau <me@ttaylorr.com>
Thanks,
Taylor
^ permalink raw reply
* [PATCH 0/1] *** EDITED add.c ***
From: Naomi Ibe @ 2023-10-09 1:18 UTC (permalink / raw)
To: git; +Cc: Naomi Ibe
*** BLURB HERE ***
Naomi (1):
[OUTREACHY] Fixed add.c file to conform to guidelines when using die()
listed in issue #635
builtin/add.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
--
2.36.1.windows.1
^ permalink raw reply
* [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Naomi Ibe @ 2023-10-09 1:16 UTC (permalink / raw)
To: git; +Cc: Naomi
From: Naomi <naomi.ibeh69@gmail.com>
Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
builtin/add.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
if (repo_read_index(the_repository) < 0)
- die(_("Could not read the index"));
+ die(_("could not read the index"));
repo_init_revisions(the_repository, &rev, prefix);
rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
die(_("editing patch failed"));
if (stat(file, &st))
- die_errno(_("Could not stat '%s'"), file);
+ die_errno(_("could not stat '%s'"), file);
if (!st.st_size)
- die(_("Empty patch. Aborted."));
+ die(_("empty patch. aborted"));
child.git_cmd = 1;
strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
NULL);
if (run_command(&child))
- die(_("Could not apply '%s'"), file);
+ die(_("could not apply '%s'"), file);
unlink(file);
free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
finish:
if (write_locked_index(&the_index, &lock_file,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("Unable to write new index file"));
+ die(_("unable to write new index file"));
dir_clear(&dir);
clear_pathspec(&pathspec);
--
2.36.1.windows.1
^ permalink raw reply related
* [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635
From: Naomi Ibe @ 2023-10-09 1:15 UTC (permalink / raw)
To: git; +Cc: Naomi
From: Naomi <naomi.ibeh69@gmail.com>
Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
builtin/add.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
if (repo_read_index(the_repository) < 0)
- die(_("Could not read the index"));
+ die(_("could not read the index"));
repo_init_revisions(the_repository, &rev, prefix);
rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
die(_("editing patch failed"));
if (stat(file, &st))
- die_errno(_("Could not stat '%s'"), file);
+ die_errno(_("could not stat '%s'"), file);
if (!st.st_size)
- die(_("Empty patch. Aborted."));
+ die(_("empty patch. aborted"));
child.git_cmd = 1;
strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
NULL);
if (run_command(&child))
- die(_("Could not apply '%s'"), file);
+ die(_("could not apply '%s'"), file);
unlink(file);
free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
finish:
if (write_locked_index(&the_index, &lock_file,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("Unable to write new index file"));
+ die(_("unable to write new index file"));
dir_clear(&dir);
clear_pathspec(&pathspec);
--
2.36.1.windows.1
^ permalink raw reply related
* Re: [OUTREACHY] git send-email issues
From: Naomi Ibe @ 2023-10-08 22:37 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <e22b6211-8534-417f-a647-e36cb00eee61@app.fastmail.com>
Oh wow
I read about gitgadget but didn't understand how it works, I'd
definitely check it out with these links you sent and I really hope it
works. Thank you very very much
On Sun, Oct 8, 2023 at 11:17 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
>
> Hi Naomi
>
> On Sun, Oct 8, 2023, at 23:59, Naomi Ibe wrote:
> > I've used the --smtp-debug tag, checked the official docs and other
> > docs too, plus stackoverflow,google and even chatgpt, but nothing
> > seems to be working. I've even had to change my gmail password tonight
> > but it still doesn't work. Any tips at all would be greatly
> > appreciated at this point. Thank you
>
> Here [1] is a good resource for setting up Gmail for git-send-email. It's
> a bit of a chore to set up but that resource was enough for me to get it
> working. One of the things that you are going to need is an App password.
>
> But have you considered using GitGitGadget instead?[2] You can make a pull
> request on that repository and then the program (gitgitgadget) can send
> the emails to this mailing list for you.
>
> [1] https://stackoverflow.com/a/68238913/1725151
>
> [2] https://github.com/gitgitgadget/gitgitgadget/
>
> --
> Kristoffer
^ permalink raw reply
* Re: [OUTREACHY] git send-email issues
From: Kristoffer Haugsbakk @ 2023-10-08 22:16 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2xBNMMTSCSsVFe3M3jFN0m0pZ-j_TAz3r6xmQrgdRujfQ@mail.gmail.com>
Hi Naomi
On Sun, Oct 8, 2023, at 23:59, Naomi Ibe wrote:
> I've used the --smtp-debug tag, checked the official docs and other
> docs too, plus stackoverflow,google and even chatgpt, but nothing
> seems to be working. I've even had to change my gmail password tonight
> but it still doesn't work. Any tips at all would be greatly
> appreciated at this point. Thank you
Here [1] is a good resource for setting up Gmail for git-send-email. It's
a bit of a chore to set up but that resource was enough for me to get it
working. One of the things that you are going to need is an App password.
But have you considered using GitGitGadget instead?[2] You can make a pull
request on that repository and then the program (gitgitgadget) can send
the emails to this mailing list for you.
[1] https://stackoverflow.com/a/68238913/1725151
[2] https://github.com/gitgitgadget/gitgitgadget/
--
Kristoffer
^ permalink raw reply
* [OUTREACHY] git send-email issues
From: Naomi Ibe @ 2023-10-08 21:59 UTC (permalink / raw)
To: git
I've been able to edit the add.c file since friday and I've created my
patches too. The error I keep getting when I run either "git
send-email --to=naomi.ibeh69@gmail.com
builtin/0001-OUTREACHY-Fixed-add.c-file-to-conform-to-guidelines-.patch"
or "git send-email --to=git@vger.kernel.org
builtin/0001-OUTREACHY-Fixed-add.c-file-to-conform-to-guidelines-.patch"
is this:
**Unable to initialize SMTP properly. Check config and use
--smtp-debug. VALUES: server=smtp.gmail.com encryption=tls
hello=localhost.localdomain port=587 at C:/Program
Files/Git/mingw64/libexec/git-core\git-send-email line 1725, <FIN>
line 1.**
I've used the --smtp-debug tag, checked the official docs and other
docs too, plus stackoverflow,google and even chatgpt, but nothing
seems to be working. I've even had to change my gmail password tonight
but it still doesn't work. Any tips at all would be greatly
appreciated at this point. Thank you
^ permalink raw reply
* Bug: 'git diff' crashes on corrupted repository
From: Dominik Paulus @ 2023-10-08 20:27 UTC (permalink / raw)
To: git
What did you do before the bug happened?
I have a git repository which is stored in a 'Seafile' shared folder[0]. As I
noticed, 'Seafile' seems to not correctly sync hidden files/dotfiles, so after
modifying the git repository on one host, the ".git" directory in the Seafile
replica on the other host is inconsistent: Specifically, it seems to be
missing a subset of the files in '.git/objects'.
'git fsck' complains about various missing blobs, dangling commits, dangling
trees, broken links, and notably also "error: HEAD: invalid sha1 pointer
[...]".
On this - obviously corrupted - repository, most git commands report "fatal:
bad object HEAD". In contrast, 'git diff' crashes:
$ git status
fatal: bad object HEAD
$ git log
fatal: bad object HEAD
$ git diff
Segmentation fault (core dumped)
$
This is the stacktrace for this crash (gdb output):
-----
Starting program: /usr/bin/git diff
Downloading separate debug info for system-supplied DSO at 0x7ffff7fc8000
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
[Detaching after fork from child process 16789]
Program received signal SIGSEGV, Segmentation fault.
0x00005555556ff6ac in diff_add_if_missing (r=0x555555966ac0 <the_repo>, to_fetch=0x7fffffffce10, filespec=0x555555975)
at /usr/src/debug/git/git-2.42.0/diff.c:6895
6895 if (filespec && filespec->oid_valid &&
(gdb) bt
#0 0x00005555556ff6ac in diff_add_if_missing (r=0x555555966ac0 <the_repo>, to_fetch=0x7fffffffce10, filespec=0x555555975)
at /usr/src/debug/git/git-2.42.0/diff.c:6895
#1 0x00005555556ff7aa in diff_queued_diff_prefetch (repository=0x555555966ac0 <the_repo>)
at /usr/src/debug/git/git-2.42.0/diff.c:6911
#2 0x00005555556f5e74 in diff_populate_filespec (r=0x555555966ac0 <the_repo>, s=0x555555977660, options=0x7fffffffcfe0)
at /usr/src/debug/git/git-2.42.0/diff.c:4184
#3 0x00005555556fee70 in diff_filespec_check_stat_unmatch (r=0x555555966ac0 <the_repo>, p=p@entry=0x5555559755e0)
at /usr/src/debug/git/git-2.42.0/diff.c:6839
#4 0x0000555555709560 in diffcore_skip_stat_unmatch (diffopt=0x7fffffffdb20) at /usr/src/debug/git/git-2.42.0/diff.c:6857
#5 diffcore_std (options=<optimized out>) at /usr/src/debug/git/git-2.42.0/diff.c:6946
#6 0x00005555556ee59c in run_diff_files (revs=<optimized out>, option=<optimized out>)
at /usr/src/debug/git/git-2.42.0/diff-lib.c:272
#7 0x00005555555ba95b in builtin_diff_files (argv=<optimized out>, argc=<optimized out>, revs=<optimized out>)
at builtin/diff.c:293
#8 cmd_diff (argc=1, argv=0x7fffffffe7a0, prefix=<optimized out>) at builtin/diff.c:586
#9 0x00005555555767f4 in run_builtin (argv=0x7fffffffe7a0, argc=1, p=0x555555938db0 <commands.lto_priv+816>)
at /usr/src/debug/git/git-2.42.0/git.c:469
#10 handle_builtin (argc=1, argv=0x7fffffffe7a0) at /usr/src/debug/git/git-2.42.0/git.c:723
#11 0x0000555555576dab in run_argv (argcp=0x7fffffffe4ec, argv=0x7fffffffe510) at /usr/src/debug/git/git-2.42.0/git.c:787
#12 0x00005555555727e3 in cmd_main (argv=<optimized out>, argc=<optimized out>) at /usr/src/debug/git/git-2.42.0/git.c:922
#13 main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/git/git-2.42.0/common-main.c:62
(gdb) print filespec->oid_valid
Cannot access memory at address 0x555555975
(gdb)
-----
What did you expect to happen?
'git diff' should not crash/perform invalid memory accesses on a corrupted repository.
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.5-arch1-1 #1 SMP PREEMPT_DYNAMIC Sat, 23 Sep 2023 22:55:13 +0000 x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.38
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
[0] I'm aware that this is a bad idea in the first place and a red flag about
the underlying workflow :).
^ permalink raw reply
* [PATCH] pretty: fix ref filtering for %(decorate) formats
From: Andy Koppe @ 2023-10-08 20:23 UTC (permalink / raw)
To: git; +Cc: Andy Koppe
Mark pretty formats containing "%(decorate" as requiring decoration in
userformat_find_requirements(), same as "%d" and "%D".
Without this, cmd_log_init_finish() didn't invoke load_ref_decorations()
with the decoration_filter it puts together, and hence filtering options
such as --decorate-refs were quietly ignored.
Amend one of the %(decorate) checks in t4205-log-pretty-formats.sh to
test this.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
pretty.c | 4 ++++
t/t4205-log-pretty-formats.sh | 6 +++---
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/pretty.c b/pretty.c
index 7f3abb676c..cf964b060c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1961,6 +1961,10 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)
case 'D':
w->decorate = 1;
break;
+ case '(':
+ if (starts_with(fmt + 1, "decorate"))
+ w->decorate = 1;
+ break;
}
}
}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 16626e4fe9..5aabc9f7d8 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -590,9 +590,9 @@ test_expect_success 'pretty format %decorate' '
git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 &&
test_cmp expect2 actual2 &&
- echo "[ HEAD -> foo; tag: bar; qux ]" >expect3 &&
- git log --format="%(decorate:prefix=[ ,suffix= ],separator=%x3B )" \
- -1 >actual3 &&
+ echo "[ bar; qux; foo ]" >expect3 &&
+ git log --format="%(decorate:prefix=[ ,suffix= ],separator=%x3B ,tag=)" \
+ --decorate-refs=refs/ -1 >actual3 &&
test_cmp expect3 actual3 &&
# Try with a typo (in "separator"), in which case the placeholder should
--
2.42.GIT
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2023, #03; Fri, 6)
From: René Scharfe @ 2023-10-08 17:34 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqqh6n24zf1.fsf@gitster.g>
Am 07.10.23 um 10:20 schrieb Junio C Hamano:
> * rs/parse-options-value-int (2023-09-18) 2 commits
> - parse-options: use and require int pointer for OPT_CMDMODE
> - parse-options: add int value pointer to struct option
>
> A bit of type safety for the "value" pointer used in the
> parse-options API.
>
> What's the status of this thing?
> source: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
tl;dr: Feel free to drop this topic if it's in the way.
I think the added _int variables are defensible, but the mode_int member
added to struct resume_mode in builtin/am.c is too ugly. Extending
OPT_CMDMODE to support arguments would allow getting rid of the struct
altogether and is probably a good idea anyway. It's surprisingly hard
to get right, though -- just discovered that the interference detection
is only working half of the time in the current code, and we can't have
that, can we?
# a OPT_CMDMODE option
$ t/helper/test-tool parse-options --mode1 | grep integer
integer: 1
# something else setting the same variable
$ % t/helper/test-tool parse-options --set23 | grep integer
integer: 23
# combined use is flagged if the OPT_CMDMODE option comes last
$ t/helper/test-tool parse-options --set23 --mode1
error: option `mode1' : incompatible with something else
# ... but not the other way around
$ t/helper/test-tool parse-options --mode1 --set23 | grep integer
integer: 23
Anyway, I'll get there eventually and present a nicely shaven yak -- or
give up and focus on the original topic.
https://lore.kernel.org/git/6cb09270-04b9-456e-8d7e-97137e56e9e2@web.de/
detects and handles the same type mismatch by adding a level of
abstraction (getters and setters). That way is more general and adds
more overhead compared to the _int variant. A fair comparison requires
argument support in OPT_CMDMODE, though, I think.
I don't mind removing this topic from seen for now; it's not ready, yet.
René
^ permalink raw reply
* Re: [PATCH 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Jeff King @ 2023-10-08 17:33 UTC (permalink / raw)
To: Taylor Blau; +Cc: Elijah Newren, Junio C Hamano, git, Eric W. Biederman
In-Reply-To: <ZSLS9G1lHruig48a@nand.local>
On Sun, Oct 08, 2023 at 12:04:04PM -0400, Taylor Blau wrote:
> > I was interested in the same question as Junio, but from a different
> > angle. fast-import documentation points out that the packs it creates
> > are suboptimal with poorer delta choices. Are the packs created by
> > bulk-checkin prone to the same issues? When I was thinking in terms
> > of having "git merge" use fast-import for pack creation instead of
> > writing loose objects (an idea I never investigated very far), I was
> > wondering if I'd need to mark those packs as "less optimal" and do
> > something to make sure they were more likely to be repacked.
> >
> > I believe geometric repacking didn't exist back when I was thinking
> > about this, and perhaps geometric repacking automatically handles
> > things nicely for us. Does it, or are we risking retaining
> > sub-optimal deltas from the bulk-checkin code?
> >
> > (I've never really cracked open the pack code, so I have absolutely no
> > idea; I'm just curious.)
>
> Yes, the bulk-checkin mechanism suffers from an even worse problem which
> is the pack it creates will contain no deltas whatsoever. The contents
> of the pack are just getting written as-is, so there's no fancy
> delta-ficiation going on.
I wonder how big a deal this would be in practice for merges.
pack-objects will look for deltas between any two candidates objects,
but in practice I think most deltas are between objects from multiple
commits (across the "time" dimension, if you will) rather than within a
single tree (the "space" dimension). And a merge operation is generally
creating a single new tree (recursive merging may create intermediate
states which would delta, but we don't actually need to keep those
intermediate ones. I won't be surprised if we do, though).
We should be able to test that theory by looking at existing deltas.
Here's a script which builds an index of blobs and trees to the commits
that introduce them:
git rev-list HEAD |
git diff-tree --stdin -r -m -t --raw |
perl -lne '
if (/^[0-9a-f]/) {
$commit = $_;
} elsif (/^:\S+ \S+ \S+ (\S+)/) {
$h{$1} = $commit;
}
END { print "$_ $h{$_}" for keys(%h) }
' >commits.db
And then we can see which deltas come from the same commit:
git cat-file --batch-all-objects --batch-check='%(objectname) %(deltabase)' |
perl -alne '
BEGIN {
open(my $fh, "<", "commits.db");
%commit = map { chomp; split } <$fh>;
}
next if $F[1] =~ /0{40}/; # not a delta
next unless defined $commit{$F[0]}; # not in index
print $commit{$F[0]} eq $commit{$F[1]} ? "inner" : "outer", " ", $_;
'
In git.git, I see 460 "inner" deltas, and 193,081 "outer" ones. The
inner ones are mostly small single-file test vectors, which makes sense.
It's possible to have a merge result that does conflict resolution in
two such files (that would then delta), but it seems like a fairly
unlikely case. Numbers for linux.git are similar.
So it might just not be a big issue at all for this use case.
> I think Michael Haggerty (?) suggested to me off-list that it might be
> interesting to have a flag that we could mark packs with bad/no deltas
> as such so that we don't implicitly trust their contents as having high
> quality deltas.
I was going to suggest the same thing. ;) Unfortunately it's a bit
tricky to do as we have no room in the file format for an optional flag.
You'd have to add a ".mediocre-delta" file or something.
But here's another approach. I recall discussing a while back the idea
that we should not necessarily trust the quality of deltas in packs that
are pushed (and I think Thomas Gummerer even did some experiments inside
GitHub with those, though I don't remember the results). And one way
around that is during geometric repacking to consider the biggest/oldest
pack as "preferred", reuse its deltas, but always compute from scratch
with the others (neither reusing on-disk deltas, nor skipping
try_delta() when two objects come from the same pack).
That same strategy would work here (and for incremental fast-import
packs, though of course not if your fast-import pack is the "big" one
after you do a from-scratch import).
> > A couple of the comments earlier in the series suggested this was
> > about streaming blobs to a pack in the bulk checkin code. Are tree
> > and commit objects also put in the pack, or will those continue to be
> > written loosely?
>
> This covers both blobs and trees, since IIUC that's all we'd need to
> implement support for merge-tree to be able to write any objects it
> creates into a pack. AFAIK merge-tree never generates any commit
> objects. But teaching 'merge' to perform the same bulk-checkin trick
> would just require us implementing index_bulk_commit_checkin_in_core()
> or similar, which is straightforward to do on top of the existing code.
This is a bit of a devil's advocate question, but: would it make sense
to implement this as a general of Git's object-writing code, and not tie
it to a specific command? That is, what if a user could do:
git --write-to-pack merge ...
but also:
git --write-to-pack add ...
and the object-writing code would just write to a pack instead of
writing loose objects. That lets the caller decide when it is or is not
a good idea to use this mode. And if making loose objects gives bad
performance for merges, wouldn't the same be true of other operations
which generate many objects?
Possibly it exacerbates the "no deltas" issue from above (though it
would depend on the command). The bigger question to me is one of
checkpointing. When do we finish off the pack with a .idx and make it
available to other readers? We could do it at program exit, but I
suspect there are some commands that really want to make objects
available sooner (e.g., as soon as "git add" writes an index, we'd want
those objects to already be available). Probably every program that
writes objects would need to be annotated with a checkpoint call (which
would be a noop in loose object mode).
So maybe it's a dumb direction. I dunno.
-Peff
^ permalink raw reply
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