* Re:[PATCH 1/1] add: Enable attr pathspec magic for git-add.
From: Joanna Wang @ 2023-10-07 0:50 UTC (permalink / raw)
To: jojwang; +Cc: git, gitster
In-Reply-To: <20231007002811.2337315-1-jojwang@google.com>
>+test_expect_success 'check that attr magic works for git' '
>+'
Sorry, i forgot to take this out. I can fix it in the next pathset after
the next round of review.
^ permalink raw reply
* [PATCH 1/1] add: Enable attr pathspec magic for git-add.
From: Joanna Wang @ 2023-10-07 0:28 UTC (permalink / raw)
To: gitster; +Cc: git, jojwang
In-Reply-To: <xmqqttr3adgg.fsf@gitster.g>
This lets users limit added files or exclude files based on file
attributes. For example, the chromium project would like to use
this like "git add --all ':(exclude,attr:submodule)'", as submodules
are managed in a unique way and often results in submodule changes
that users do not want in their commits.
This does not change any attr magic implementation. It is only adding
attr as an allowed pathspec in git-add, which was previously
blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
With this patch, attr is supported. It is possible that when the attr
pathspec feature was first added in
b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
"PATHSPEC_ATTR" was just unintentionally left out of a few
GUARD_PATHSPEC() invocations. Later, to get a more user-friendly error
message when attr was used with git-add, PATHSPEC_ATTR was added as a
mask to git-add's invocation of parse_pathspec()
84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
git-stash which goes through the same GUARD_PATHSPEC(), currently
does not work with attr. So a PATHSPEC_ATTR mask has been added
to its parse_pathspec and parse_pathspec_file()
Signed-off-by: Joanna Wang <jojwang@google.com>
---
> (perhaps except for the commit title on the Subject: header).
Added 'add:' and specified it is 'patchpsec' magic, but lmk if i'm still missing something.
> This is rather a poor statement to make, as it hints that there are
> known breakages this change will reveal that you are not telling us,
>although I suspect that it is not the case.
Yes, sorry I did not mean to make this hint. I was trying to express that
if bugs did come up, we would not abandon this effort and expect someone
else to fix it. I have removed this line. I realized I also don't want
to say we will forever be on the hook for fixing every related bug that comes up.
> These are good things to add to the new test this patch adds.
Tests added.
>Hmph, is it a good idea in general to use ALL_MAGIC in guard?
My original reasoning was using ALL_MAGIC would prevent new magic pathspecs
from being unintentionally left out of commands and if a new magic does not
work with a command, whoever adds it should change ALL_MAGIC back to listing
them individually in the same patch. But yes, listing them individually seems
safer. Better to accidentally leave magic out that can be added with intention
later rather than make it easy to accidentally leave unsupported magic in.
I switched it back to listing individually.
>It is strongly preferrable, instead of butchering this test that
>guards these two mechanisms from being broken, to find a command
>that still has some restriction on the magic it allows, and use it
>to make sure they still trigger and give "magic not supported" error
>message.
I added the test back with git-stash, which, after more testing i discovered it
actually did not fully work with attr. More info below.
>Unless it matters that these files have recent timestamps, do not
>use "touch", merely to ensure presence of a file. We often use a
>simple redirection.
Done
>A tangent (to the topic of testing, but relevant to the whole
>patch). I notice 'stash' is mentioned on the topic, but I do not
>see changes to the codepath that is specific to 'stash', and changes
>to the tests do not demonstrate existing breakage. The lack of code
>changes probably is because something shared, which is pretected
>with magic guard lifted by the patch, is called from 'stash' as well
>as 'add', or something?
Yes, the previous patch enabled attr with git-stash which was only blocked
at the shared dir.c GUARD_PATHSPEC level. I thought attr was working for git-stash
but I was wrong after a few more manual tests.
So I added PATHSPEC_ATTR as a mask to stash's parse_pathspec and parse_pathspec_file.
builtin/add.c | 7 ++-
builtin/stash.c | 4 +-
dir.c | 13 ++--
t/t6135-pathspec-with-attrs.sh | 109 +++++++++++++++++++++++++++++++--
4 files changed, 117 insertions(+), 16 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..2de83964a3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
* Check the "pathspec '%s' did not match any files" block
* below before enabling new magic.
*/
- parse_pathspec(&pathspec, PATHSPEC_ATTR,
+ parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH,
prefix, argv);
@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (pathspec.nr)
die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
- parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
+ parse_pathspec_file(&pathspec, 0,
PATHSPEC_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH,
prefix, pathspec_from_file, pathspec_file_nul);
@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
PATHSPEC_LITERAL |
PATHSPEC_GLOB |
PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE);
+ PATHSPEC_EXCLUDE |
+ PATHSPEC_ATTR);
for (i = 0; i < pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
diff --git a/builtin/stash.c b/builtin/stash.c
index 1ad496985a..9c77d3e4e4 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1760,7 +1760,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
}
}
- parse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
+ parse_pathspec(&ps, PATHSPEC_ATTR, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
prefix, argv);
if (pathspec_from_file) {
@@ -1773,7 +1773,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
if (ps.nr)
die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
- parse_pathspec_file(&ps, 0,
+ parse_pathspec_file(&ps, PATHSPEC_ATTR,
PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,
prefix, pathspec_from_file, pathspec_file_nul);
} else if (pathspec_file_nul) {
diff --git a/dir.c b/dir.c
index 8486e4d56f..9bf9b53ca5 100644
--- a/dir.c
+++ b/dir.c
@@ -2174,12 +2174,13 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
return 0;
GUARD_PATHSPEC(pathspec,
- PATHSPEC_FROMTOP |
- PATHSPEC_MAXDEPTH |
- PATHSPEC_LITERAL |
- PATHSPEC_GLOB |
- PATHSPEC_ICASE |
- PATHSPEC_EXCLUDE);
+ PATHSPEC_FROMTOP |
+ PATHSPEC_MAXDEPTH |
+ PATHSPEC_LITERAL |
+ PATHSPEC_GLOB |
+ PATHSPEC_ICASE |
+ PATHSPEC_EXCLUDE |
+ PATHSPEC_ATTR);
for (i = 0; i < pathspec->nr; i++) {
const struct pathspec_item *item = &pathspec->items[i];
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index f70c395e75..531b4f4d5e 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
fileSetLabel label
fileValue label=foo
fileWrongLabel label☺
+ newFileA* labelA
+ newFileB* labelB
EOF
echo fileSetLabel label1 >sub/.gitattributes &&
git add .gitattributes sub/.gitattributes &&
git commit -m "add attributes"
'
+test_expect_success 'setup .gitignore' '
+ cat <<-\EOF >.gitignore &&
+ actual
+ expect
+ pathspec_file
+ EOF
+ git add .gitignore &&
+ git commit -m "add gitignore"
+'
+
test_expect_success 'check specific set attr' '
cat <<-\EOF >expect &&
fileSetLabel
@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
test_expect_success 'check unspecified attr' '
cat <<-\EOF >expect &&
.gitattributes
+ .gitignore
fileA
fileAB
fileAC
@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
test_expect_success 'check unspecified attr (2)' '
cat <<-\EOF >expect &&
HEAD:.gitattributes
+ HEAD:.gitignore
HEAD:fileA
HEAD:fileAB
HEAD:fileAC
@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
test_expect_success 'check multiple unspecified attr' '
cat <<-\EOF >expect &&
.gitattributes
+ .gitignore
fileC
fileNoLabel
fileWrongLabel
@@ -239,16 +254,100 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
test_i18ngrep "Only one" actual
'
-test_expect_success 'fail if attr magic is used places not implemented' '
+test_expect_success 'fail if attr magic is used in places not implemented' '
# The main purpose of this test is to check that we actually fail
# when you attempt to use attr magic in commands that do not implement
- # attr magic. This test does not advocate git-add to stay that way,
- # though, but git-add is convenient as it has its own internal pathspec
- # parsing.
- test_must_fail git add ":(attr:labelB)" 2>actual &&
+ # attr magic. This test does not advocate stash push to stay that way.
+ # When you teach the command to grok the pathspec, you need to find
+ # another commnad to replace it for the test.
+ test_must_fail git stash push ":(attr:labelB)" 2>actual &&
+ test_i18ngrep "magic not supported" actual
+'
+
+test_expect_success 'fail if attr magic is used in --pathspec-from-file when not implemented' '
+ # This is like the test above but for attr magic pass in via --pathspec-from-file.
+ cat <<-\EOF >pathspec_file &&
+ :(attr:labelB)
+ EOF
+ test_must_fail git stash push --pathspec-from-file=pathspec_file 2>actual &&
test_i18ngrep "magic not supported" actual
'
+test_expect_success 'check that attr magic works for git add --all' '
+ cat <<-\EOF >expect &&
+ sub/newFileA-foo
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ git add --all ":(exclude,attr:labelB)" &&
+ git diff --name-only --cached >actual &&
+ git restore -W -S . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add -u' '
+ cat <<-\EOF >expect &&
+ sub/fileA
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ >sub/fileA &&
+ >sub/fileB &&
+ git add -u ":(exclude,attr:labelB)" &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . && rm sub/new* &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add <path>' '
+ cat <<-\EOF >expect &&
+ fileA
+ fileB
+ sub/fileA
+ EOF
+ >fileA &&
+ >fileB &&
+ >sub/fileA &&
+ >sub/fileB &&
+ git add ":(exclude,attr:labelB)sub/*" &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git -add .' '
+ cat <<-\EOF >expect &&
+ sub/fileA
+ EOF
+ >fileA &&
+ >fileB &&
+ >sub/fileA &&
+ >sub/fileB &&
+ cd sub &&
+ git add . ":(exclude,attr:labelB)" &&
+ cd .. &&
+ git diff --name-only --cached >actual &&
+ git restore -S -W . &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
+ cat <<-\EOF >pathspec_file &&
+ :(exclude,attr:labelB)
+ EOF
+ cat <<-\EOF >expect &&
+ sub/newFileA-foo
+ EOF
+ >sub/newFileA-foo &&
+ >sub/newFileB-foo &&
+ git add --all --pathspec-from-file=pathspec_file &&
+ git diff --name-only --cached >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'check that attr magic works for git' '
+'
+
test_expect_success 'abort on giving invalid label on the command line' '
test_must_fail git ls-files . ":(attr:☺)"
'
--
2.42.0.609.gbb76f46606-goog
^ permalink raw reply related
* Re: [OUTREACHY] Permission To Work On Tasks
From: Junio C Hamano @ 2023-10-06 23:28 UTC (permalink / raw)
To: Naomi Ibe; +Cc: git
In-Reply-To: <CACS=G2zsJxP+NWuosZyrFGctJptHNYTrULErRo_Ns41KeMuMqA@mail.gmail.com>
Naomi Ibe <naomi.ibeh69@gmail.com> writes:
> First issue is here https://github.com/gitgitgadget/git/issues/635 ,
> involving changing the "die()" error msg outputs to all lowercase. I
> found a few files here https://github.com/git/git/tree/master/builtin
> where the "die()" error msg had some uppercase in them (add.c in lines
> 185, 203, 205, 211 and 571) (branch.c in lines 521, 525, 581, 597,
> 599, 627, 629, 643, 650, 652, 776, 926, 954 and 968). If I'm allowed
> to work on this issue, how many files should I edit? The last closed
> issues related to this issue had edited five files.
As the "general microproject information" page says, it is a good
idea to do just one quality-focused microproject per applicant.
If I were on the receiving end to review such a patch, I would
probably find it is too boring a burden if it had several unrelated
commands covered by a single patch, and would stop reading in the
middle.
If I were on the sending end to work on them for real (not as "dip
my toe in the water and say hello to the more experienced
developers" exercise), I would probably prepare a series of patches,
one for each git subcommand (e.g. "add", "branch", "log", etc.), and
for shared infrastructure files, one for each subsystem that they
are part of (this is harder to do for a new person who do not know
what subsystems exist, and which files implement which subsystem),
but for a microproject, I would say a single file under builtin/*
hierarchy would be a good size.
> Second issue is this https://github.com/gitgitgadget/git/issues/302 .
> Is it still available to be worked on? I notice it was opened in 2019
Stepping back a bit, do you agree with what the issue says?
Remember, these "issue"s are merely one person's opinion and not
endorsed by the community.
Before you ask "is it still available", do you know the current
status (not the status of the "issue")? Have you looked at "git
commit --help" to find it out yourself to see if "now" is singled
out? Here is what we say in our documentation:
In addition to recognizing all date formats above, the --date
option will also try to make sense of other, more human-centric
date formats, such as relative dates like "yesterday" or "last
Friday at noon".
So apparently it is still "available". It is a different matter how
well a patch that adds "now" to the examples listed there will be
accepted, though. During a microproject, one of the things new
contributors are expected to learn is to convince others the cause
of their patches with the proposed commit log message well.
Finally, you do not need to obtain permission to work on anything
around here. You work on what interests you, send the result (or
send request for help, to which others may offer advices if the
problem you are solving looks interesting) to be reviewed, and will
be thanked for working on it when your patch is applied. To avoid
duplicated work, you might want to say "I'm interested in doing
this, but is anybody already doing it? If so I'll avoid stepping on
their toes", but otherwise, you are expected to go wild on your own
;-)
Have fun.
^ permalink raw reply
* Re: [PATCH v3 1/3] diff-merges: improve --diff-merges documentation
From: Junio C Hamano @ 2023-10-06 23:19 UTC (permalink / raw)
To: Sergey Organov; +Cc: Elijah Newren, git
In-Reply-To: <875y3jr42h.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Elijah Newren <newren@gmail.com> writes:
>>
>>>> > +--cc::
>>>> > + Produce dense combined diff output for merge commits.
>>>> > + Shortcut for '--diff-merges=dense-combined -p'.
>>>>
>>>> Good.
>>>>
>>>> > +--remerge-diff::
>>>> > + Produce diff against re-merge.
>>>> > + Shortcut for '--diff-merges=remerge -p'.
>>>> ...
>>> Perhaps:
>>>
>>> Produce remerge-diff output for merge commits, in order to show how
>>> conflicts were resolved.
>>
>> I do not mind it, but then I'd prefer to see ", in order to show
>> how" also in the description of "--cc" and "-c" for consistency.
>>
>> A succinct way to say what they do may be hard to come by, but I
>> think of them showing places that did not have obvious natural
>> resolution.
>
> So, is it OK with both of you if I leave it as:
>
> "Produce remerge-diff output for merge commits."
>
> for now, and let you tweak the descriptions later on, if needed?
I do not know what Elijah would say, but in one of iterations of my
draft response to him indeed suggested that "in order to" here is
not necessary if it is described for the "--diff-merges=remerge"
option, because those who know enough to skip referring to the other
entry are expected to know why it exists. So I think I am OK with
that.
Thanks.
^ permalink raw reply
* Re: [PATCH 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-06 23:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, Eric W. Biederman, Jeff King
In-Reply-To: <xmqqil7j751u.fsf@gitster.g>
On Fri, Oct 06, 2023 at 03:35:25PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > 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.
>
> Well, be it "git merge-tree" or "git merge", new loose objects tend
> to accumulate until "gc" kicks in, so it is not a new problem for
> mere mortals, is it?
Yeah, I would definitely suspect that this is more of an issue for
forges than individual Git users.
> As one "interesting" use case of "merge-tree" is for a Git hosting
> site with bare repositories to offer trial merges, without which
> majority of the object their repositories acquire would have been in
> packs pushed by their users, "Gee, loose objects consume many inodes
> in exchange for easier selective pruning" becomes an issue, right?
Right.
> Just like it hurts performance to have too many loose object files,
> presumably it would also hurt performance to keep too many packs,
> each came from such a trial merge. Do we have a "gc" story offered
> for these packs created by the new feature? E.g., "once merge-tree
> is done creating a trial merge, we can discard the objects created
> in the pack, because we never expose new objects in the pack to the
> outside, processes running simultaneously, so instead closing the
> new packfile by calling flush_bulk_checkin_packfile(), we can safely
> unlink the temporary pack. We do not even need to spend cycles to
> run a gc that requires cycles to enumerate what is still reachable",
> or something like that?
I know Johannes worked on something like this recently. IIRC, it
effectively does something like:
struct tmp_objdir *tmp_objdir = tmp_objdir_create(...);
tmp_objdir_replace_primary_odb(tmp_objdir, 1);
at the beginning of a merge operation, and:
tmp_objdir_discard_objects(tmp_objdir);
at the end. I haven't followed that work off-list very closely, but it
is only possible for GitHub to discard certain niche kinds of
merges/rebases, since in general we make the objects created during test
merges available via refs/pull/N/{merge,rebase}.
I think that like anything, this is a trade-off. Having lots of packs
can be a performance hindrance just like having lots of loose objects.
But since we can represent more objects with fewer inodes when packed,
storing those objects together in a pack is preferable when (a) you're
doing lots of test-merges, and (b) you want to keep those objects
around, e.g., because they are reachable.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v7 2/3] unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-10-06 22:58 UTC (permalink / raw)
To: phillip.wood; +Cc: Junio C Hamano, git, linusa, calvinwan, rsbecker
In-Reply-To: <0b6de919-8dbf-454f-807b-5abb64388cb7@gmail.com>
On 2023.09.24 14:57, phillip.wood123@gmail.com wrote:
> On 22/09/2023 21:05, Junio C Hamano wrote:
> > Any thought on the "polarity" of the return values from the
> > assertion? I still find it confusing and hard to follow.
>
> When I was writing this I was torn between whether to follow our usual
> convention of returning zero for success and minus one for failure or to
> return one for success and zero for failure. In the end I decided to go with
> the former but I tend to agree with you that the latter would be easier to
> understand.
Agreed. V8 will switch to 0 for failure and 1 for success for the TEST,
TEST_TODO, and check macros.
> > > > +test_expect_success 'TAP output from unit tests' '
> > > > [...]
> > > > + ok 19 - test with no checks returns -1
> > > > + 1..19
> > > > + EOF
> > >
> > > Presumably t-basic will serve as a catalog of check_* functions and
> > > the test binary, together with this test piece, will keep growing as
> > > we gain features in the unit tests infrastructure. I wonder how
> > > maintainable the above is, though. When we acquire new test, we
> > > would need to renumber. What if multiple developers add new
> > > features to the catalog at the same time?
>
> I think we could just add new tests to the end so we'd only need to change
> the "1..19" line. That will become a source of merge conflicts if multiple
> developers add new features at the same time though. Having several unit
> test programs called from separate tests in t0080 might help with that.
My hope is that test-lib.c will not have to grow too extensively after
this series; that said, it's already been a pain to have to adjust the
t0080 expected text several times just during development of this
series. I'll look into splitting this into several "meta-tests", but I'm
not sure I'll get to it for V8 yet.
> > > > diff --git a/t/unit-tests/.gitignore b/t/unit-tests/.gitignore
> > > > new file mode 100644
> > > > index 0000000000..e292d58348
> > > > --- /dev/null
> > > > +++ b/t/unit-tests/.gitignore
> > > > @@ -0,0 +1,2 @@
> > > > +/t-basic
> > > > +/t-strbuf
> > >
> > > Also, can we come up with some naming convention so that we do not
> > > have to keep adding to this file every time we add a new test
> > > script?
>
> Perhaps we should put the unit test binaries in a separate directory so we
> can just add that directory to .gitignore.
Sounds good to me.
^ permalink raw reply
* [PATCH] doc: update list archive reference to use lore.kernel.org
From: Junio C Hamano @ 2023-10-06 22:57 UTC (permalink / raw)
To: git
No disrespect to other mailing list archives, but the local part of
their URLs will become pretty much meaningless once the archives go
out of service, and we learned the lesson hard way when $gmane
stopped serving.
Let's point into https://lore.kernel.org/ for an article that can be
found there, because the local part of the URL has the Message-Id:
that can be used to find the same message in other archives, even if
lore goes down.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/CodingGuidelines | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 65af8d82ce..71afc5b259 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -24,7 +24,7 @@ code. For Git in general, a few rough rules are:
"Once it _is_ in the tree, it's not really worth the patch noise to
go and fix it up."
- Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
+ Cf. https://lore.kernel.org/all/20100126160632.3bdbe172.akpm@linux-foundation.org/
- Log messages to explain your changes are as important as the
changes themselves. Clearly written code and in-code comments
--
2.42.0-325-g3a06386e31
^ permalink raw reply related
* [OUTREACHY] Permission To Work On Tasks
From: Naomi Ibe @ 2023-10-06 22:41 UTC (permalink / raw)
To: git
So I went through this link
https://github.com/gitgitgadget/git/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22
and I found two issues that interest me.
First issue is here https://github.com/gitgitgadget/git/issues/635 ,
involving changing the "die()" error msg outputs to all lowercase. I
found a few files here https://github.com/git/git/tree/master/builtin
where the "die()" error msg had some uppercase in them (add.c in lines
185, 203, 205, 211 and 571) (branch.c in lines 521, 525, 581, 597,
599, 627, 629, 643, 650, 652, 776, 926, 954 and 968). If I'm allowed
to work on this issue, how many files should I edit? The last closed
issues related to this issue had edited five files.
Second issue is this https://github.com/gitgitgadget/git/issues/302 .
Is it still available to be worked on? I notice it was opened in 2019
^ permalink raw reply
* Re: [PATCH 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Junio C Hamano @ 2023-10-06 22:35 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Elijah Newren, Eric W. Biederman, Jeff King
In-Reply-To: <e96921014557edb41dd73d93a8c3cf6cfaf0c719.1696629697.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> 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.
Well, be it "git merge-tree" or "git merge", new loose objects tend
to accumulate until "gc" kicks in, so it is not a new problem for
mere mortals, is it?
As one "interesting" use case of "merge-tree" is for a Git hosting
site with bare repositories to offer trial merges, without which
majority of the object their repositories acquire would have been in
packs pushed by their users, "Gee, loose objects consume many inodes
in exchange for easier selective pruning" becomes an issue, right?
Just like it hurts performance to have too many loose object files,
presumably it would also hurt performance to keep too many packs,
each came from such a trial merge. Do we have a "gc" story offered
for these packs created by the new feature? E.g., "once merge-tree
is done creating a trial merge, we can discard the objects created
in the pack, because we never expose new objects in the pack to the
outside, processes running simultaneously, so instead closing the
new packfile by calling flush_bulk_checkin_packfile(), we can safely
unlink the temporary pack. We do not even need to spend cycles to
run a gc that requires cycles to enumerate what is still reachable",
or something like that?
Thanks.
^ permalink raw reply
* Re: [PATCH 4/4] files-backend.c: avoid stat in 'loose_fill_ref_dir'
From: Junio C Hamano @ 2023-10-06 22:12 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <e193a45318244d9f8b05dfe2fb1ce57f6a4f6428.1696615769.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Unlike other existing usage of 'get_dtype', the 'follow_symlinks' arg is set
> to 1 to replicate the existing handling of symlink dirents. This
> unfortunately requires calling 'stat' on the associated entry regardless of
> platform, but symlinks in the loose ref store are highly unlikely since
> they'd need to be created manually by a user.
Yeek. I wonder what breaks if we do not do this follow_symlinks()
part, i.e., either just replace stat() with lstat() in the original
without any of these four patches (which would be simple to figure
out what breaks), or omit [3/4] and let get_dtype() yield DT_LNK.
It seems that it comes from a7e66ae3 ([PATCH] Make do_each_ref()
follow symlinks., 2005-08-16), and just like I commented on there in
its log message back then, I still doubt that following a symbolic
link is a great idea here in this codepath.
But optimization without behaviour change is a good way to ensure
that optimization does not introduce new bugs, and because keeping
the historical behaviour like the patches [3/4] and this patch does
is more work (meaning: if it proves unnecessary to dereference
symbolic links, we can remove code instead of having to write new
code to support the new behaviour), let's take the series as-is, and
defer it to future developers to further clean-up the semantics.
> Note that this patch also changes the condition for skipping creation of a
> ref entry from "when 'stat' fails" to "when the d_type is anything other
> than DT_REG or DT_DIR". If a dirent's d_type is DT_UNKNOWN (either because
> the platform doesn't support d_type in dirents or some other reason) or
> DT_LNK, 'get_dtype' will try to derive the underlying type with 'stat'. If
> the 'stat' fails, the d_type will remain 'DT_UNKNOWN' and dirent will be
> skipped. However, it will also be skipped if it is any other valid d_type
> (e.g. DT_FIFO for named pipes, DT_LNK for a nested symlink). Git does not
> handle these properly anyway, so we can safely constrain accepted types to
> directories and regular files.
Sounds good.
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> refs/files-backend.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
Thanks.
^ permalink raw reply
* [PATCH 7/7] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-10-06 22:02 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1696629697.git.me@ttaylorr.com>
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 | 43 +++++++++++----
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++++++++++++++++++++++
5 files changed, 136 insertions(+), 10 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 0de42aecf4..672ebd4c54 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -18,6 +18,7 @@
#include "quote.h"
#include "tree.h"
#include "config.h"
+#include "bulk-checkin.h"
static int line_termination = '\n';
@@ -414,6 +415,7 @@ struct merge_tree_options {
int show_messages;
int name_only;
int use_stdin;
+ int write_pack;
};
static int real_merge(struct merge_tree_options *o,
@@ -440,6 +442,7 @@ static int real_merge(struct merge_tree_options *o,
init_merge_options(&opt, the_repository);
opt.show_rename_progress = 0;
+ opt.write_pack = o->write_pack;
opt.branch1 = branch1;
opt.branch2 = branch2;
@@ -548,6 +551,8 @@ 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_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 8631c99700..85d8c5c6b3 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
@@ -2124,11 +2125,19 @@ static int handle_content_merge(struct merge_options *opt,
if ((merge_status < 0) || !result_buf.ptr)
ret = err(opt, _("Failed to execute internal merge"));
- if (!ret &&
- write_object_file(result_buf.ptr, result_buf.size,
- OBJ_BLOB, &result->oid))
- ret = err(opt, _("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 = err(opt, _("Unable to add %s to database"),
+ path);
+ }
free(result_buf.ptr);
if (ret)
@@ -3618,7 +3627,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)
@@ -3652,8 +3662,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;
}
@@ -3818,8 +3834,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;
}
@@ -4353,9 +4369,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);
@@ -4899,6 +4919,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 b88000e3c2..156e160876 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 250f721795..2d81ff4de5 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -922,4 +922,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 &&
+
+ 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.8.g7a7e1e881e.dirty
^ permalink raw reply related
* [PATCH 6/7] bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-06 22:02 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1696629697.git.me@ttaylorr.com>
The remaining missing piece in order to teach the `merge-tree` builtin
how to write the contents of a merge into a pack is a function to index
tree objects into a bulk-checkin pack.
This patch implements that missing piece, which is a thin wrapper around
all of the functionality introduced in previous commits.
If and when Git gains support for a "compatibility" hash algorithm, the
changes to support that here will be minimal. The bulk-checkin machinery
will need to convert the incoming tree to compute its length under the
compatibility hash, necessary to reconstruct its header. With that
information (and the converted contents of the tree), the bulk-checkin
machinery will have enough to keep track of the converted object's hash
in order to update the compatibility mapping.
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.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 25 +++++++++++++++++++++++++
bulk-checkin.h | 4 ++++
2 files changed, 29 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 319921efe7..d7d46f1dac 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -418,6 +418,20 @@ static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
flags);
}
+static int deflate_tree_to_pack_incore(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ git_hash_ctx ctx;
+
+ format_object_header_hash(the_hash_algo, &ctx, OBJ_TREE, size);
+
+ return deflate_obj_contents_to_pack_incore(state, &ctx, result_oid,
+ buf, size, OBJ_TREE, path,
+ flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -508,6 +522,17 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,
return status;
}
+int index_tree_bulk_checkin_incore(struct object_id *oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ int status = deflate_tree_to_pack_incore(&bulk_checkin_packfile, oid,
+ buf, size, path, 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 1b91daeaee..89786b3954 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -17,6 +17,10 @@ int index_blob_bulk_checkin_incore(struct object_id *oid,
const void *buf, size_t size,
const char *path, unsigned flags);
+int index_tree_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.8.g7a7e1e881e.dirty
^ permalink raw reply related
* [PATCH 5/7] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Taylor Blau @ 2023-10-06 22:02 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1696629697.git.me@ttaylorr.com>
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.
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_blob_to_pack_incore()`, which is
responsible for formatting the pack header and then deflating the
contents into the pack. The latter is accomplished by calling
deflate_blob_contents_to_pack_incore(), which takes advantage of the
earlier refactoring and is responsible for writing the object to the
pack and handling any overage from pack.packSizeLimit.
The bulk of the new functionality is implemented in the function
`stream_obj_to_pack_incore()`, which is a generic implementation for
writing objects of arbitrary type (whose contents we can fit in-core)
into a bulk-checkin pack.
The new function shares an unfortunate degree of similarity to the
existing `stream_blob_to_pack()` function. But DRY-ing up these two
would likely be more trouble than it's worth, since the latter has to
deal with reading and writing the contents of the object.
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 | 116 +++++++++++++++++++++++++++++++++++++++++++++++++
bulk-checkin.h | 4 ++
2 files changed, 120 insertions(+)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index a9497fcb28..319921efe7 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -140,6 +140,69 @@ static int already_written(struct bulk_checkin_packfile *state, struct object_id
return 0;
}
+static int stream_obj_to_pack_incore(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx,
+ off_t *already_hashed_to,
+ const void *buf, size_t size,
+ enum object_type type,
+ const char *path, unsigned flags)
+{
+ git_zstream s;
+ unsigned char obuf[16384];
+ unsigned hdrlen;
+ int status = Z_OK;
+ int write_object = (flags & HASH_WRITE_OBJECT);
+
+ git_deflate_init(&s, pack_compression_level);
+
+ hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
+ s.next_out = obuf + hdrlen;
+ s.avail_out = sizeof(obuf) - hdrlen;
+
+ if (*already_hashed_to < size) {
+ size_t hsize = size - *already_hashed_to;
+ if (hsize) {
+ the_hash_algo->update_fn(ctx, buf, hsize);
+ }
+ *already_hashed_to = size;
+ }
+ s.next_in = (void *)buf;
+ s.avail_in = size;
+
+ while (status != Z_STREAM_END) {
+ status = git_deflate(&s, Z_FINISH);
+ if (!s.avail_out || status == Z_STREAM_END) {
+ if (write_object) {
+ size_t written = s.next_out - obuf;
+
+ /* would we bust the size limit? */
+ if (state->nr_written &&
+ pack_size_limit_cfg &&
+ pack_size_limit_cfg < state->offset + written) {
+ git_deflate_abort(&s);
+ return -1;
+ }
+
+ hashwrite(state->f, obuf, written);
+ state->offset += written;
+ }
+ s.next_out = obuf;
+ s.avail_out = sizeof(obuf);
+ }
+
+ switch (status) {
+ case Z_OK:
+ case Z_BUF_ERROR:
+ case Z_STREAM_END:
+ continue;
+ default:
+ die("unexpected deflate failure: %d", status);
+ }
+ }
+ git_deflate_end(&s);
+ return 0;
+}
+
/*
* Read the contents from fd for size bytes, streaming it to the
* packfile in state while updating the hash in ctx. Signal a failure
@@ -313,6 +376,48 @@ static void finalize_checkpoint(struct bulk_checkin_packfile *state,
}
}
+static int deflate_obj_contents_to_pack_incore(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ enum object_type type,
+ const char *path, unsigned flags)
+{
+ struct hashfile_checkpoint checkpoint = {0};
+ struct pack_idx_entry *idx = NULL;
+ off_t already_hashed_to = 0;
+
+ /* Note: idx is non-NULL when we are writing */
+ if (flags & HASH_WRITE_OBJECT)
+ CALLOC_ARRAY(idx, 1);
+
+ while (1) {
+ prepare_checkpoint(state, &checkpoint, idx, flags);
+ if (!stream_obj_to_pack_incore(state, ctx, &already_hashed_to,
+ buf, size, type, path, flags))
+ break;
+ truncate_checkpoint(state, &checkpoint, idx);
+ }
+
+ finalize_checkpoint(state, ctx, &checkpoint, idx, result_oid);
+
+ return 0;
+}
+
+static int deflate_blob_to_pack_incore(struct bulk_checkin_packfile *state,
+ struct object_id *result_oid,
+ const void *buf, size_t size,
+ const char *path, unsigned flags)
+{
+ git_hash_ctx ctx;
+
+ format_object_header_hash(the_hash_algo, &ctx, OBJ_BLOB, size);
+
+ return deflate_obj_contents_to_pack_incore(state, &ctx, result_oid,
+ buf, size, OBJ_BLOB, path,
+ flags);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -392,6 +497,17 @@ 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_blob_to_pack_incore(&bulk_checkin_packfile, oid,
+ buf, size, path, 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.8.g7a7e1e881e.dirty
^ permalink raw reply related
* [PATCH 4/7] bulk-checkin: factor our `finalize_checkpoint()`
From: Taylor Blau @ 2023-10-06 22:01 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1696629697.git.me@ttaylorr.com>
In a similar spirit as previous commits, factor out the routine to
finalize the just-written object from the bulk-checkin mechanism.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 41 +++++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 16 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 2dae8be461..a9497fcb28 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -289,6 +289,30 @@ static void truncate_checkpoint(struct bulk_checkin_packfile *state,
flush_bulk_checkin_packfile(state);
}
+static void finalize_checkpoint(struct bulk_checkin_packfile *state,
+ git_hash_ctx *ctx,
+ struct hashfile_checkpoint *checkpoint,
+ struct pack_idx_entry *idx,
+ struct object_id *result_oid)
+{
+ the_hash_algo->final_oid_fn(result_oid, ctx);
+ if (!idx)
+ return;
+
+ idx->crc32 = crc32_end(state->f);
+ if (already_written(state, result_oid)) {
+ hashfile_truncate(state->f, checkpoint);
+ state->offset = checkpoint->offset;
+ free(idx);
+ } else {
+ oidcpy(&idx->oid, result_oid);
+ ALLOC_GROW(state->written,
+ state->nr_written + 1,
+ state->alloc_written);
+ state->written[state->nr_written++] = idx;
+ }
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -320,22 +344,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
return error("cannot seek back");
}
- the_hash_algo->final_oid_fn(result_oid, &ctx);
- if (!idx)
- return 0;
-
- idx->crc32 = crc32_end(state->f);
- if (already_written(state, result_oid)) {
- hashfile_truncate(state->f, &checkpoint);
- state->offset = checkpoint.offset;
- free(idx);
- } else {
- oidcpy(&idx->oid, result_oid);
- ALLOC_GROW(state->written,
- state->nr_written + 1,
- state->alloc_written);
- state->written[state->nr_written++] = idx;
- }
+ finalize_checkpoint(state, &ctx, &checkpoint, idx, result_oid);
return 0;
}
--
2.42.0.8.g7a7e1e881e.dirty
^ permalink raw reply related
* [PATCH 3/7] bulk-checkin: factor out `truncate_checkpoint()`
From: Taylor Blau @ 2023-10-06 22:01 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1696629697.git.me@ttaylorr.com>
In a similar spirit as previous commits, factor our the routine to
truncate a bulk-checkin packfile when writing past the pack size limit.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 377c41f3ad..2dae8be461 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -273,6 +273,22 @@ static void prepare_checkpoint(struct bulk_checkin_packfile *state,
}
}
+static void truncate_checkpoint(struct bulk_checkin_packfile *state,
+ struct hashfile_checkpoint *checkpoint,
+ struct pack_idx_entry *idx)
+{
+ /*
+ * Writing this object to the current pack will make
+ * it too big; we need to truncate it, start a new
+ * pack, and write into it.
+ */
+ if (!idx)
+ BUG("should not happen");
+ hashfile_truncate(state->f, checkpoint);
+ state->offset = checkpoint->offset;
+ flush_bulk_checkin_packfile(state);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -300,16 +316,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
fd, size, path, flags))
break;
- /*
- * Writing this object to the current pack will make
- * it too big; we need to truncate it, start a new
- * pack, and write into it.
- */
- if (!idx)
- BUG("should not happen");
- hashfile_truncate(state->f, &checkpoint);
- state->offset = checkpoint.offset;
- flush_bulk_checkin_packfile(state);
+ truncate_checkpoint(state, &checkpoint, idx);
if (lseek(fd, seekback, SEEK_SET) == (off_t) -1)
return error("cannot seek back");
}
--
2.42.0.8.g7a7e1e881e.dirty
^ permalink raw reply related
* [PATCH 2/7] bulk-checkin: factor out `prepare_checkpoint()`
From: Taylor Blau @ 2023-10-06 22:01 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1696629697.git.me@ttaylorr.com>
In a similar spirit as the previous commit, factor out the routine to
prepare streaming into a bulk-checkin pack into its own function. Unlike
the previous patch, this is a verbatim copy and paste.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 0aac3dfe31..377c41f3ad 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -260,6 +260,19 @@ static void format_object_header_hash(const struct git_hash_algo *algop,
algop->update_fn(ctx, header, header_len);
}
+static void prepare_checkpoint(struct bulk_checkin_packfile *state,
+ struct hashfile_checkpoint *checkpoint,
+ struct pack_idx_entry *idx,
+ unsigned flags)
+{
+ prepare_to_stream(state, flags);
+ if (idx) {
+ hashfile_checkpoint(state->f, checkpoint);
+ idx->offset = state->offset;
+ crc32_begin(state->f);
+ }
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -283,12 +296,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
already_hashed_to = 0;
while (1) {
- prepare_to_stream(state, flags);
- if (idx) {
- hashfile_checkpoint(state->f, &checkpoint);
- idx->offset = state->offset;
- crc32_begin(state->f);
- }
+ prepare_checkpoint(state, &checkpoint, idx, flags);
if (!stream_blob_to_pack(state, &ctx, &already_hashed_to,
fd, size, path, flags))
break;
--
2.42.0.8.g7a7e1e881e.dirty
^ permalink raw reply related
* [PATCH 1/7] bulk-checkin: factor out `format_object_header_hash()`
From: Taylor Blau @ 2023-10-06 22:01 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
In-Reply-To: <cover.1696629697.git.me@ttaylorr.com>
Before deflating a blob into a pack, the bulk-checkin mechanism prepares
the pack object header by calling `format_object_header()`, and writing
into a scratch buffer, the contents of which eventually makes its way
into the pack.
Future commits will add support for deflating multiple kinds of objects
into a pack, and will likewise need to perform a similar operation as
below.
This is a mostly straightforward extraction, with one notable exception.
Instead of hard-coding `the_hash_algo`, pass it in to the new function
as an argument. This isn't strictly necessary for our immediate purposes
here, but will prove useful in the future if/when the bulk-checkin
mechanism grows support for the hash transition plan.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
bulk-checkin.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 223562b4e7..0aac3dfe31 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -247,6 +247,19 @@ static void prepare_to_stream(struct bulk_checkin_packfile *state,
die_errno("unable to write pack header");
}
+static void format_object_header_hash(const struct git_hash_algo *algop,
+ git_hash_ctx *ctx, enum object_type type,
+ size_t size)
+{
+ unsigned char header[16384];
+ unsigned header_len = format_object_header((char *)header,
+ sizeof(header),
+ type, size);
+
+ algop->init_fn(ctx);
+ algop->update_fn(ctx, header, header_len);
+}
+
static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
struct object_id *result_oid,
int fd, size_t size,
@@ -254,8 +267,6 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
{
off_t seekback, already_hashed_to;
git_hash_ctx ctx;
- unsigned char obuf[16384];
- unsigned header_len;
struct hashfile_checkpoint checkpoint = {0};
struct pack_idx_entry *idx = NULL;
@@ -263,10 +274,7 @@ static int deflate_blob_to_pack(struct bulk_checkin_packfile *state,
if (seekback == (off_t) -1)
return error("cannot find the current offset");
- header_len = format_object_header((char *)obuf, sizeof(obuf),
- OBJ_BLOB, size);
- the_hash_algo->init_fn(&ctx);
- the_hash_algo->update_fn(&ctx, obuf, header_len);
+ format_object_header_hash(the_hash_algo, &ctx, OBJ_BLOB, size);
/* Note: idx is non-NULL when we are writing */
if ((flags & HASH_WRITE_OBJECT) != 0)
--
2.42.0.8.g7a7e1e881e.dirty
^ permalink raw reply related
* [PATCH 0/7] merge-ort: implement support for packing objects together
From: Taylor Blau @ 2023-10-06 22:01 UTC (permalink / raw)
To: git; +Cc: Elijah Newren, Eric W. Biederman, Jeff King, Junio C Hamano
(Based on the tip of 'eb/limit-bulk-checkin-to-blobs'.)
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.
The motivating use-case behind these changes is to better support
repositories who invoke merge-tree frequently, generating a potentially
large number of loose objects, resulting in a possible adverse effect on
performance.
The majority of the changes here are preparatory to refactor common
routines out of the bulk-checkin machinery to prepare for indexing
different types of objects whose contents can be held in-core.
Also worth noting is the relative ease this series can be adapted to
support the $NEW_HASH interop work in 'eb/hash-transition-rfc'. For more
details on the relatively small number of changes necessary to make that
work, see the log message of the second-to-last patch.
Thanks in advance for your review!
Taylor Blau (7):
bulk-checkin: factor out `format_object_header_hash()`
bulk-checkin: factor out `prepare_checkpoint()`
bulk-checkin: factor out `truncate_checkpoint()`
bulk-checkin: factor our `finalize_checkpoint()`
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 | 249 ++++++++++++++++++++++++++-----
bulk-checkin.h | 8 +
merge-ort.c | 43 ++++--
merge-recursive.h | 1 +
t/t4301-merge-tree-write-tree.sh | 93 ++++++++++++
7 files changed, 355 insertions(+), 48 deletions(-)
--
2.42.0.8.g7a7e1e881e.dirty
^ permalink raw reply
* Re: [PATCH 2/4] dir.[ch]: expose 'get_dtype'
From: Junio C Hamano @ 2023-10-06 22:00 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <24014010ea350a2ea8676b6560ca1d60838c56ef.1696615769.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> Move 'get_dtype()' from 'diagnose.c' to 'dir.c' and add its declaration to
> 'dir.h' so that it is accessible to callers in other files. The function and
> its documentation are moved verbatim except for a small addition to the
> description clarifying what the 'path' arg represents.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> diagnose.c | 36 ------------------------------------
> dir.c | 28 ++++++++++++++++++++++++++++
> dir.h | 11 +++++++++++
> 3 files changed, 39 insertions(+), 36 deletions(-)
OK. diagnose.c should still have access to the function as it
includes <dir.h>, and to anybody that includes <dir.h> and sees the
declaration of get_dtype(), DT_FOO should be visible because <dir.h>
includes <statinfo.h> that has fallback definition of DT_FOO.
Looking simple and straight-forward.
> diff --git a/diagnose.c b/diagnose.c
> index 8430064000b..fc4d344bd63 100644
> --- a/diagnose.c
> +++ b/diagnose.c
> @@ -71,42 +71,6 @@ static int dir_file_stats(struct object_directory *object_dir, void *data)
> return 0;
> }
>
> -/*
> - * Get the d_type of a dirent. If the d_type is unknown, derive it from
> - * stat.st_mode.
> - *
> - * Note that 'path' is assumed to have a trailing slash. It is also modified
> - * in-place during the execution of the function, but is then reverted to its
> - * original value before returning.
> - */
> -static unsigned char get_dtype(struct dirent *e, struct strbuf *path)
> -{
> - struct stat st;
> - unsigned char dtype = DTYPE(e);
> - size_t base_path_len;
> -
> - if (dtype != DT_UNKNOWN)
> - return dtype;
> -
> - /* d_type unknown in dirent, try to fall back on lstat results */
> - base_path_len = path->len;
> - strbuf_addstr(path, e->d_name);
> - if (lstat(path->buf, &st))
> - goto cleanup;
> -
> - /* determine d_type from st_mode */
> - if (S_ISREG(st.st_mode))
> - dtype = DT_REG;
> - else if (S_ISDIR(st.st_mode))
> - dtype = DT_DIR;
> - else if (S_ISLNK(st.st_mode))
> - dtype = DT_LNK;
> -
> -cleanup:
> - strbuf_setlen(path, base_path_len);
> - return dtype;
> -}
> -
> static int count_files(struct strbuf *path)
> {
> DIR *dir = opendir(path->buf);
> diff --git a/dir.c b/dir.c
> index 8486e4d56ff..5e01af3a25e 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2235,6 +2235,34 @@ static int get_index_dtype(struct index_state *istate,
> return DT_UNKNOWN;
> }
>
> +unsigned char get_dtype(struct dirent *e, struct strbuf *path)
> +{
> + struct stat st;
> + unsigned char dtype = DTYPE(e);
> + size_t base_path_len;
> +
> + if (dtype != DT_UNKNOWN)
> + return dtype;
> +
> + /* d_type unknown in dirent, try to fall back on lstat results */
> + base_path_len = path->len;
> + strbuf_addstr(path, e->d_name);
> + if (lstat(path->buf, &st))
> + goto cleanup;
> +
> + /* determine d_type from st_mode */
> + if (S_ISREG(st.st_mode))
> + dtype = DT_REG;
> + else if (S_ISDIR(st.st_mode))
> + dtype = DT_DIR;
> + else if (S_ISLNK(st.st_mode))
> + dtype = DT_LNK;
> +
> +cleanup:
> + strbuf_setlen(path, base_path_len);
> + return dtype;
> +}
> +
> static int resolve_dtype(int dtype, struct index_state *istate,
> const char *path, int len)
> {
> diff --git a/dir.h b/dir.h
> index ad06682fd54..28c630ce806 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -363,6 +363,17 @@ struct dir_struct {
>
> struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp);
>
> +/*
> + * Get the d_type of a dirent. If the d_type is unknown, derive it from
> + * stat.st_mode using the path to the dirent's containing directory (path) and
> + * the name of the dirent itself.
> + *
> + * Note that 'path' is assumed to have a trailing slash. It is also modified
> + * in-place during the execution of the function, but is then reverted to its
> + * original value before returning.
> + */
> +unsigned char get_dtype(struct dirent *e, struct strbuf *path);
> +
> /*Count the number of slashes for string s*/
> int count_slashes(const char *s);
^ permalink raw reply
* Re: [PATCH 1/4] ref-cache.c: fix prefix matching in ref iteration
From: Junio C Hamano @ 2023-10-06 21:51 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <59276a5b3fd1fd3b25db73e096cf0e834af2d4f9.1696615769.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> Update 'cache_ref_iterator_advance' to skip over refs that are not matched
> by the given prefix.
>
> Currently, a ref entry is considered "matched" if the entry name is fully
> contained within the prefix:
>
> * prefix: "refs/heads/v1"
> * entry: "refs/heads/v1.0"
>
> OR if the prefix is fully contained in the entry name:
>
> * prefix: "refs/heads/v1.0"
> * entry: "refs/heads/v1"
>
> The first case is always correct, but the second is only correct if the ref
> cache entry is a directory, for example:
>
> * prefix: "refs/heads/example"
> * entry: "refs/heads/"
>
> Modify the logic in 'cache_ref_iterator_advance' to reflect these
> expectations:
>
> 1. If 'overlaps_prefix' returns 'PREFIX_EXCLUDES_DIR', then the prefix and
> ref cache entry do not overlap at all. Skip this entry.
> 2. If 'overlaps_prefix' returns 'PREFIX_WITHIN_DIR', then the prefix matches
> inside this entry if it is a directory. Skip if the entry is not a
> directory, otherwise iterate over it.
> 3. Otherwise, 'overlaps_prefix' returned 'PREFIX_CONTAINS_DIR', indicating
> that the cache entry (directory or not) is fully contained by or equal to
> the prefix. Iterate over this entry.
>
> Note that condition 2 relies on the names of directory entries having the
> appropriate trailing slash. The existing function documentation of
> 'create_dir_entry' explicitly calls out the trailing slash requirement, so
> this is a safe assumption to make.
Thanks for explaining it very well and clearly.
Allowing prefix="refs/heads/v1.0" to yield entry="refs/heads/v1"
(case #2 above that this patch fixes the behaviour for) would cause
ref_iterator_advance() to return a ref outside the hierarhcy,
wouldn't it? So it appears to me that either one of the two would
be true:
* the code is structured in such a way that such a condition does
not actually happen (in which case this patch would be a no-op),
or
* there is a bug in the current code that is fixed by this patch,
whose externally observable behaviour can be verified with a
test.
It is not quite clear to me which is the case here. The code with
the patch looks more logical than the original, but I am not sure
how to demonstrate the existing breakage (if any).
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> refs/ref-cache.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index 2294c4564fb..6e3b725245c 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -412,7 +412,8 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
>
> if (level->prefix_state == PREFIX_WITHIN_DIR) {
> entry_prefix_state = overlaps_prefix(entry->name, iter->prefix);
> - if (entry_prefix_state == PREFIX_EXCLUDES_DIR)
> + if (entry_prefix_state == PREFIX_EXCLUDES_DIR ||
> + (entry_prefix_state == PREFIX_WITHIN_DIR && !(entry->flag & REF_DIR)))
> continue;
> } else {
> entry_prefix_state = level->prefix_state;
^ permalink raw reply
* Re: [PATCH 0/4] Performance improvement & cleanup in loose ref iteration
From: Junio C Hamano @ 2023-10-06 19:09 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <pull.1594.git.1696615769.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> While investigating ref iteration performance in builtins like
> 'for-each-ref' and 'show-ref', I found two small improvement opportunities.
>
> The first patch tweaks the logic around prefix matching in
> 'cache_ref_iterator_advance' so that we correctly skip refs that do not
> actually match a given prefix. The unnecessary iteration doesn't seem to be
> causing any bugs in the ref iteration commands that I've tested, but it
> doesn't hurt to be more precise (and it helps with some other patches I'm
> working on ;) ).
>
> The next three patches update how 'loose_fill_ref_dir' determines the type
> of ref cache entry to create (directory or regular). On platforms that
> include d_type information in 'struct dirent' (as far as I can tell, all
> except NonStop & certain versions of Cygwin), this allows us to skip calling
> 'stat'. In ad-hoc testing, this improved performance of 'git for-each-ref'
> by about 20%.
Yay. That is a very obvious one, once it is pointed out. Thanks
for noticing and improving. Looking forward to reading the patches
themselves.
^ permalink raw reply
* Re: How To Pick And Work On A Microproject
From: Junio C Hamano @ 2023-10-06 19:03 UTC (permalink / raw)
To: Christian Couder; +Cc: git, Naomi Ibe, Kaartic Sivaraam
In-Reply-To: <CAP8UFD1cd5YZqAxYbYUMNkAYJLLGjBpNe_NK5nVq3eLxxSDzEQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> I am not sure how others feel about this, but I think it would be
> better in the future to not have to prepare such pages, and to just
> have a section with a number of examples of good microprojects on this
> https://git.github.io/General-Microproject-Information/ page. It will
> be easier to update this section when we know about other good ideas
> or better ideas, or when we want to remove an idea that we don't
> consider good anymore, or just update an idea.
If we have curated one-stop shop for microproject candidates to make
it easy to find them, it would be a vast improvement over the status
quo. The easier for us to update the contents of the list, the
better for participants. Having only one place that we need to look
at is one way to do so, and the general microproject information
page would be the best place to host it. I like it.
>> Then it goes on to suggest finding a bug report, but I tend to think
>> that fixing them is way oversized to be a good microproject.
>
> I agree that it's oversized for most bugs. I have just added the
> following paragraph at the end of this "Searching for bug reports"
> subsection:
>
> "Also some bugs are difficult to understand and require too much or
> too difficult work for a microproject, so don’t spend too much time on
> them if it appears that they might not be simple to fix, and don’t
> hesitate to ask on the mailing list if they are a good microproject."
Would that be better, or would it be simpler to gut the whole
paragraph about bug reports? This is "how to pick a microproject",
not "how to pick your main project to work on during your mentoring
program".
Unlike #leftoverbits that sometimes cover trivial but boring style
normalization and easy refactoring of code into helper functions, I
have never seen a bug report on the list that may make a good
microproject. If we were to add a curated list of microproject idea
on the general microproject information page, it probably is better
to remove these mentions of bugreports and #leftoverbits, so that
readers will not get distracted. "Don't hesitate to ask" so that
they may try to tackle more challenging one, if they wish, is a good
thing to say nevertheless.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 3/3] completion: complete '--dd'
From: Sergey Organov @ 2023-10-06 18:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqlecgeoan.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> '--dd' only makes sense for 'git log' and 'git show', so add it to
>> __git_log_show_options which is referenced in the completion for these
>> two commands.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>> contrib/completion/git-completion.bash | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 133ec92bfae7..ca4fa39f3ff8 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -2042,7 +2042,7 @@ __git_log_shortlog_options="
>> "
>> # Options accepted by log and show
>> __git_log_show_options="
>> - --diff-merges --diff-merges= --no-diff-merges --remerge-diff
>> + --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
>> "
>>
>> __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
>
> Quite straight-forward. I am kind of surprised that we do not have
> to list "--cc" here. Perhaps it is so short and common that people
> do not need completion help?
>
> But that is not a new problem caused by this series, so it is OK.
>
> Unless "--cc" gets completed without being listed here, using some
> automation like the "--git-completion-helper" option, in which case
> we may want to see if we can remove all of the above and complete
> them the same way as "--cc" gets completed. I didn't check.
I checked, though with rather old 2.25.1 running on my Ubuntu, and it
is not completed.
I think that it's still a good idea to add --cc to completions, so that
it's there in the suggested completion list, for the sake of
discoverability. That's why I bothered to add --dd to the completions.
Thanks,
-- Sergey Organov
>
> Thanks.
^ permalink raw reply
* Re: [PATCH v3 1/3] diff-merges: improve --diff-merges documentation
From: Sergey Organov @ 2023-10-06 18:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq34yog3ux.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
[...]
>> +on, m::
>> + Make diff output for merge commits to be shown in the default
>> + format. The default format could be changed using
>> `log.diffMerges` configuration parameter, which default value
>> is `separate`.
>
> The original is already wrong so these are not problems this patch
> introduces, but
>
> - "configuration variable" is how we refer to these entities.
> - "which default value" -> "whose default value".
Added this amendment to the patch.
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: [PATCH v3 1/3] diff-merges: improve --diff-merges documentation
From: Sergey Organov @ 2023-10-06 18:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, git
In-Reply-To: <xmqq7cnzaav0.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Elijah Newren <newren@gmail.com> writes:
>
>>> > +--cc::
>>> > + Produce dense combined diff output for merge commits.
>>> > + Shortcut for '--diff-merges=dense-combined -p'.
>>>
>>> Good.
>>>
>>> > +--remerge-diff::
>>> > + Produce diff against re-merge.
>>> > + Shortcut for '--diff-merges=remerge -p'.
>>> ...
>> Perhaps:
>>
>> Produce remerge-diff output for merge commits, in order to show how
>> conflicts were resolved.
>
> I do not mind it, but then I'd prefer to see ", in order to show
> how" also in the description of "--cc" and "-c" for consistency.
>
> A succinct way to say what they do may be hard to come by, but I
> think of them showing places that did not have obvious natural
> resolution.
So, is it OK with both of you if I leave it as:
"Produce remerge-diff output for merge commits."
for now, and let you tweak the descriptions later on, if needed?
Thanks,
-- Sergey Organov
^ 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