* [PATCH 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
From: Shourya Shukla @ 2020-08-25 11:30 UTC (permalink / raw)
To: git
Cc: gitster, christian.couder, kaartic.sivaraam, Johannes.Schindelin,
peff, liu.denton, Shourya Shukla, Christian Couder
In-Reply-To: <20200825113020.71801-1-shouryashukla.oo@gmail.com>
The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on
Windows due to a different error message
error: cannot spawn git: No such file or directory
instead of
fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
Tighten up the check to compute '{src,dst}_abbrev' by guarding the
'verify_submodule_committish()' call using `p->status !='D'`, so that
the former isn't called in case of non-existent submodule directory,
consequently, there is no such error message on any execution
environment.
Therefore, eliminate the 'grep' check in t7421. Instead, verify the
absence of an error message by doing a 'test_must_be_empty' on the
file containing the error.
Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
---
builtin/submodule--helper.c | 7 ++++---
t/t7421-submodule-summary-add.sh | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 93d0700891..f1951680f7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,
static void generate_submodule_summary(struct summary_cb *info,
struct module_cb *p)
{
- char *displaypath, *src_abbrev, *dst_abbrev;
+ char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;
int missing_src = 0, missing_dst = 0;
char *errmsg = NULL;
int total_commits = -1;
@@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info,
}
if (S_ISGITLINK(p->mod_src)) {
- src_abbrev = verify_submodule_committish(p->sm_path,
- oid_to_hex(&p->oid_src));
+ if (p->status != 'D')
+ src_abbrev = verify_submodule_committish(p->sm_path,
+ oid_to_hex(&p->oid_src));
if (!src_abbrev) {
missing_src = 1;
/*
diff --git a/t/t7421-submodule-summary-add.sh b/t/t7421-submodule-summary-add.sh
index 59a9b00467..b070f13714 100755
--- a/t/t7421-submodule-summary-add.sh
+++ b/t/t7421-submodule-summary-add.sh
@@ -58,7 +58,7 @@ test_expect_success 'submodule summary output for submodules with changed paths'
git commit -m "change submodule path" &&
rev=$(git -C sm rev-parse --short HEAD^) &&
git submodule summary HEAD^^ -- my-subm >actual 2>err &&
- grep "fatal:.*my-subm" err &&
+ test_must_be_empty err &&
cat >expected <<-EOF &&
* my-subm ${rev}...0000000:
--
2.28.0
^ permalink raw reply related
* Re: [PATCH v2 2/3] Optionally skip linking/copying the built-ins
From: Johannes Schindelin @ 2020-08-25 8:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <xmqqmu2j50kk.fsf@gitster.c.googlers.com>
Hi Junio,
On Mon, 24 Aug 2020, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > While Git still supports the dashed form (by hard-linking the `git`
> > executable to the dashed name in `libexec/git-core/`), in practice, it
> > is probably almost irrelevant.
>
> It is irrelevant when you have to say "probably" without facts, and
> this paragraph is not necessary to justify this option. I'd omit it.
I would like to gently request to keep the sentence in, as it will provide
me with the context when I stumble across this commit the next time.
> We do care about keeping people's scripts working (even if they were
> written before Windows folks started using Git---those people who
> started using Git before that still exist ;-).
That, however, I totally understand, and I think you're right, I should
add this sentence (in one form or another).
> > In fact, some platforms (such as Windows) only started gaining
> > meaningful Git support _after_ the dashed form was deprecated, and
> > therefore one would expect that all this hard-linking is unnecessary on
> > those platforms.
> >
> > In addition to that, some programs that are regularly used to assess
> > disk usage fail to realize that those are hard-links, and heavily
> > overcount disk usage. Most notably, this was the case with Windows
> > Explorer up until the last couple of Windows 10 versions.
>
> However, the above two paragraphs I would suggest to keep, as they
> do matter---it is a good justification to have this configurable.
> Windows folks won't be able to copy and use POSIX shell scripts
> written by folks before the Windows port of Git was started to
> become widely used anyway.
Excellent!
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions
From: Johannes Schindelin @ 2020-08-25 8:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <xmqqimd750dr.fsf@gitster.c.googlers.com>
Hi Junio,
On Mon, 24 Aug 2020, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Originally, all of Git's subcommands were implemented in their own
> > executable/script, using the naming scheme `git-<command-name>`. When
> > more and more functionality was turned into built-in commands (i.e. the
> > `git` executable could run them without spawning a separate process),
> > for backwards-compatibility, we hard-link the `git` executable to
> > `git-<built-in>` for every built-in.
> >
> > This backwards-compatibility was needed to support scripts that called
> > the dashed form, even if we deprecated that a _long_ time ago.
>
> This paragraph is irrelevant. We are keeping the support for it and
> this topic is not newly deprecating or removing anything. We need
> to argue for a need to test an installation that lacks these builtin
> subcommands anywhere on disk under their own names, which you did
> succinctly below (and there is no need for "For that reason,"
> there).
Could we please keep it? It will help me in the future when stumbling over
this commit, to remember the context.
> > For that reason, we just introduced a Makefile knob to skip linking
> > them. TO make sure that this keeps working, teach the CI
>
> s/TO/To/
Thanks! I guess my keys got sticky or something ;-)
> > (and PR) builds to skip generating those hard-links.
>
> What is not justified enough is why we no longer test installations
> with dashed builtins on disk. If this topic is primarily about
> Windows (as 2/3 said), perhaps we can do this only for Windows tasks
> before we make a colletive decision to _DROP_ support for the on-disk
> builtin subcommands?
Oh, sorry, I will amend the commit message to clarify that the dashed form
is actually not tested at all anymore. Specifically since e4597aae6590
(run test suite without dashed git-commands in PATH, 2009-12-02), in fact.
All this change does is to make it an even stronger committment to run the
test suite without dashed Git commands.
Thanks,
Dscho
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > ci/run-build-and-tests.sh | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> > index 6c27b886b8..1df9402c3b 100755
> > --- a/ci/run-build-and-tests.sh
> > +++ b/ci/run-build-and-tests.sh
> > @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
> > *) ln -s "$cache_dir/.prove" t/.prove;;
> > esac
> >
> > -make
> > +make SKIP_DASHED_BUILT_INS=YesPlease
> > case "$jobname" in
> > linux-gcc)
> > make test
>
^ permalink raw reply
* pw/add-p-allowed-options-fix, was Re: What's cooking in git.git (Aug 2020, #06; Mon, 24)
From: Johannes Schindelin @ 2020-08-25 8:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq7dtn3785.fsf@gitster.c.googlers.com>
Hi Junio,
On Mon, 24 Aug 2020, Junio C Hamano wrote:
> * pw/add-p-allowed-options-fix (2020-08-17) 2 commits
> - add -p: fix checking of user input
> - add -p: use ALLOC_GROW_BY instead of ALLOW_GROW
>
> "git add -p" update.
>
> Will merge to 'next'.
I thought the consensus was to untangle the refactoring in the second
patch by first fixing the `e` case, and _then_ refactoring?
With Phillip being offline for a couple days, however, I think we can just
go forward with the patches as-are. They are in "good enough" a shape.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v3 05/11] commit-graph: return 64-bit generation number
From: Jakub Narębski @ 2020-08-25 12:18 UTC (permalink / raw)
To: Abhishek Kumar; +Cc: 85d03km98l.fsf, git, stolee
In-Reply-To: <20200825050448.GA21012@Abhishek-Arch>
Abhishek Kumar <abhishekkumar8222@gmail.com> writes:
> On Fri, Aug 21, 2020 at 03:14:34PM +0200, Jakub Narębski wrote:
>> "Abhishek Kumar via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Abhishek Kumar <abhishekkumar8222@gmail.com>
>>>
>>> In a preparatory step, let's return timestamp_t values from
>>> commit_graph_generation(), use timestamp_t for local variables
>>
>> All right, this is all good.
>>
>>> and define GENERATION_NUMBER_INFINITY as (2 ^ 63 - 1) instead.
>>
>> This needs more detailed examination. There are two similar constants,
>> GENERATION_NUMBER_INFINITY and GENERATION_NUMBER_MAX. The former is
>> used for newest commits outside the commit-graph, while the latter is
>> maximum number that commits in the commit-graph can have (because of the
>> storage limitations). We therefore need GENERATION_NUMBER_INFINITY
>> to be larger than GENERATION_NUMBER_MAX, and it is (and was).
>>
>> The GENERATION_NUMBER_INFINITY is because of the above requirement
>> traditionally taken as maximum value that can be represented in the data
>> type used to store commit's generation number _in memory_, but it can be
>> less. For timestamp_t the maximum value that can be represented
>> is (2 ^ 63 - 1).
>>
>> All right then.
>
> Related to this, by the end of this series we are using
> GENERATION_NUMBER_MAX in just one place - compute_generation_numbers()
> to make sure the topological levels fit within 30 bits.
>
> Would it be more appropriate to rename GENERATION_NUMBER_MAX to
> GENERATION_NUMBER_V1_MAX (along the lines of
> GENERATION_NUMBER_V2_OFFSET_MAX) to correctly describe that is a
> limit on topological levels, rather than generation number value?
Yes, I think that at the end of this patch series we should be using
GENERATION_NUMBER_V1_MAX and GENERATION_NUMBER_V2_OFFSET_MAX to describe
storage limits, and GENERATION_NUMBER_INFINITY (the latter as generation
number value for commits not in graph).
We need to ensure that both GENERATION_NUMBER_V1_MAX and
GENERATION_NUMBER_V2_OFFSET_MAX are smaller than
GENERATION_NUMBER_INFINITY.
However, as I wrote, handling GENERATION_NUMBER_V2_OFFSET_MAX is
difficult. As far as I can see, we can choose one of the *three*
solutions (the third one is _new_):
a. store 64-bit corrected commit date in the GDAT chunk
all possible values are able to be stored, no need for
GENERATION_NUMBER_V2_MAX,
b. store 32-bit corrected commit date offset in the GDAT chunk,
if its value is larger than GENERATION_NUMBER_V2_OFFSET_MAX,
do not write GDAT chunk at all (like for backward compatibility
with mixed-version chains of split commit-graph layers),
c. store 32-bit corrected commit date offset in the GDAT chunk,
using some kind of overflow handling scheme; for example if
the most significant bit of 32-bit value is 1, then the
rest 31-bits are position in GDOV chunk, which uses 64-bit
to store those corrected commit date offsets that do not
fit in 32 bits.
This type of schema is used in other places in Git code, if I remember
it correctly.
>> The commit message says nothing about the new symbolic constant
>> GENERATION_NUMBER_V1_INFINITY, though.
>>
>> I'm not sure it is even needed (see comments below).
>
> Yes, you are correct. I tried it out with your suggestions and it wasn't
> really needed.
>
> Thanks for catching this!
Mistakes can happen when changig how the series is split into commits.
Best,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Derrick Stolee @ 2020-08-25 12:45 UTC (permalink / raw)
To: Son Luong Ngoc, Taylor Blau; +Cc: git, dstolee
In-Reply-To: <CB6B70D3-5FC6-43FE-8460-33F6CFC123E6@gmail.com>
On 8/25/2020 3:55 AM, Son Luong Ngoc wrote:
> Hi Taylor,
>
> Thanks for working on this.
>
>> On Aug 25, 2020, at 04:01, Taylor Blau <me@ttaylorr.com> wrote:
>>
>> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'
>> learned to remove a multi-pack-index file if it added or removed a pack
>> from the object store.
>>
>> This mechanism is a little over-eager, since it is only necessary to
>> drop a MIDX if 'git repack' removes a pack that the MIDX references.
>> Adding a pack outside of the MIDX does not require invalidating the
>> MIDX, and likewise for removing a pack the MIDX does not know about.
>
> I wonder if its worth to trigger write_midx_file() to update the midx instead of
> just removing MIDX?
It would be reasonable to have 'git repack' run write_midx_file() after updating
the pack-files, but it still needs to delete the existing multi-pack-index after
deleting a referenced pack, as the current multi-pack-index will be incorrect,
leading to possibly invalid data during the write. 'git multi-pack-index expire'
carefully handles deleting pack-files that have become redundant.
It may be possible to change the behavior of write_midx_file() to check for the
state of each referenced pack-file, but it would then need to re-scan the
pack-indexes to rebuild the set of objects and which pack-files contain them.
> That is already the direction we are taking in the 'maintenance' patch series
> whenever the multi-pack-index file was deemed invalid.
The 'maintenance' builtin definitely adds new MIDX-aware maintenance tasks.
By performing a repack using the multi-pack-index as the starting point, we
can get around these issues.
One thing I've noticed by talking with Taylor about this change is that
'git repack' is a bit like 'git gc' in that it does a LOT of things and it
can be hard to understand exactly when certain sub-tasks are run or not.
There are likely some interesting operations that could be separated into
maintenance tasks. For example, 'git repack -d' is very similar to
'git maintenance run --task=loose-objects'.
> Or perhaps, we can check for 'core.multiPackIndex' value (which recently is
> 'true' by default) and determine whether we should remove the MIDX or rewrite
> it?
We currently rewrite it during 'git repack' when GIT_TEST_MULTI_PACK_INDEX=1
to maximize how often we have a multi-pack-index in the test suite. It would
be simple to update that to also check a config option. I don't think adding
that behavior to 'core.multiPackIndex' is a good direction, though.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Derrick Stolee @ 2020-08-25 13:14 UTC (permalink / raw)
To: Taylor Blau, Jeff King; +Cc: git, dstolee
In-Reply-To: <20200825023710.GA98081@syl.lan>
On 8/24/2020 10:37 PM, Taylor Blau wrote:
> On Mon, Aug 24, 2020 at 10:26:14PM -0400, Jeff King wrote:
>> On Mon, Aug 24, 2020 at 10:01:04PM -0400, Taylor Blau wrote:
>>
>>> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'
>>> learned to remove a multi-pack-index file if it added or removed a pack
>>> from the object store.
>>>
>>> This mechanism is a little over-eager, since it is only necessary to
>>> drop a MIDX if 'git repack' removes a pack that the MIDX references.
>>> Adding a pack outside of the MIDX does not require invalidating the
>>> MIDX, and likewise for removing a pack the MIDX does not know about.
>>
>> Does "git repack" ever remove just one pack? Obviously "git repack -ad"
>> or "git repack -Ad" is going to pack everything and delete the old
>> packs. So I think we'd want to remove a midx there.
>>
>> And "git repack -d" I think of as deleting only loose objects that we
>> just packed. But I guess it could also remove a pack that has now been
>> made redundant? That seems like a rare case in practice, but I suppose
>> is possible.
>
> Yeah, the patch message makes this sound more likely than it actually
> is, which I agree is very rare. I often write 'git repack' instead of
> 'git pack-objects' to slurp up everything loose into a new pack without
> having to list loose objects by name.
>
> That's the case that I really care about here: purely adding a new pack
> should not invalidate the existing MIDX.
>
>> Not exactly related to your fix, but kind of the flip side of it: would
>> we ever need to retain a midx that mentions some packs that still exist?
>>
>> E.g., imagine we have a midx that points to packs A and B, and
>> git-repack deletes B. By your logic above, we need to remove the midx
>> because now it points to objects in B which aren't accessible. But by
>> deleting it, could we be deleting the only thing that mentions the
>> objects in A?
>>
>> I _think_ the answer is "no", because we never went all-in on midx and
>> allowed deleting the matching .idx files for contained packs. So we'd
>> still have that A.idx, and we could just use the pack as normal. But
>> it's an interesting corner case if we ever do go in that direction.
>
> Agreed. Maybe a (admittedly somewhat large) #leftoverbits.
>
>> If you'll let me muse a bit more on midx-lifetime issues (which I've
>> never really thought about before just now):
>>
>> I'm also a little curious how bad it is to have a midx whose pack has
>> gone away. I guess we'd answer queries for "yes, we have this object"
>> even if we don't, which is bad. Though in practice we'd only delete
>> those packs if we have their objects elsewhere. And the pack code is
>> pretty good about retrying other copies of objects that can't be
>> accessed. Alternatively, I wonder if the midx-loading code ought to
>> check that all of the constituent packs are available.
>>
>> In that line of thinking, do we even need to delete midx files if one of
>> their packs goes away? The reading side probably ought to be able to
>> handle that gracefully.
>
> I think that this is probably the right direction, although I've only
> spend time in the MIDX code over the past couple of weeks, so I can't
> say with authority. It seems like it would be pretty annoying, though.
> For example, code that cares about listing all objects in a MIDX would
> have to check first whether the pack they're in still exists before
> emitting them. On top of that, there are more corner cases when object X
> exists in more than one pack, but some strict subset of those packs
> containing X have gone away.
>
> I don't think that it couldn't be done, though.
>
>> And the more interesting case is when you repack everything with "-ad"
>> or similar, at which point you shouldn't even need to look up what's in
>> the midx to see if you deleted its packs. The point of your operation is
>> to put it all-into-one, so you know the old midx should be discarded.
>>
>>> Teach 'git repack' to check for this by loading the MIDX, and checking
>>> whether the to-be-removed pack is known to the MIDX. This requires a
>>> slightly odd alternation to a test in t5319, which is explained with a
>>> comment.
>>
>> My above musings aside, this seems like an obvious improvement.
>>
>>> diff --git a/builtin/repack.c b/builtin/repack.c
>>> index 04c5ceaf7e..98fac03946 100644
>>> --- a/builtin/repack.c
>>> +++ b/builtin/repack.c
>>> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,
>>> static void remove_redundant_pack(const char *dir_name, const char *base_name)
>>> {
>>> struct strbuf buf = STRBUF_INIT;
>>> - strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
>>> + struct multi_pack_index *m = get_multi_pack_index(the_repository);
>>> + strbuf_addf(&buf, "%s.pack", base_name);
>>> + if (m && midx_contains_pack(m, buf.buf))
>>> + clear_midx_file(the_repository);
>>> + strbuf_insertf(&buf, 0, "%s/", dir_name);
>>
>> Makes sense. midx_contains_pack() is a binary search, so we'll spend
>> O(n log n) effort deleting the packs (I wondered if this might be
>> accidentally quadratic over the number of packs).
>
> Right. The MIDX stores packs in lexographic order, so checking them is
> O(log n), which we do at most 'n' times.
>
>> And after we clear, "m" will be NULL, so we'll do it at most once. Which
>> is why you can get rid of the manual "midx_cleared" flag from the
>> preimage.
>
> Yep. I thought briefly about passing 'm' as a parameter, but then you
> have to worry about a dangling reference to
> 'the_repository->objects->multi_pack_index' after calling
> 'clear_midx_file()', so it's easier to look it up each time.
The discussion in this thread matches my understanding of the
situation.
>> So the patch looks good to me.
The code in builtin/repack.c looks good for sure. I have a quick question
about this new test:
+test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
+ git multi-pack-index write &&
+ cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
+ test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
+
+ # Write a new pack that is unknown to the multi-pack-index.
+ git hash-object -w </dev/null >blob &&
+ git pack-objects $objdir/pack/pack <blob &&
+
+ GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
+ test_cmp_bin $objdir/pack/multi-pack-index \
+ $objdir/pack/multi-pack-index.bak
+'
+
You create an arbitrary blob, and then add it to a pack-file. Do we
know that 'git repack' is definitely creating a new pack-file that makes
our manually-created pack-file redundant?
My suggestion is to have the test check itself:
+test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
+ git multi-pack-index write &&
+ cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
+ test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
+
+ # Write a new pack that is unknown to the multi-pack-index.
+ git hash-object -w </dev/null >blob &&
+ HASH=$(git pack-objects $objdir/pack/pack <blob) &&
+
+ GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
+ test_cmp_bin $objdir/pack/multi-pack-index \
+ $objdir/pack/multi-pack-index.bak &&
+ test_path_is_missing $objdir/pack/pack-$HASH.pack
+'
+
This test fails for me, on the 'test_path_is_missing'. Likely, the
blob is seen as already in a pack-file so is just pruned by 'git repack'
instead. I thought that perhaps we need to add a new pack ourselves that
overrides the small pack. Here is my attempt:
test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
git multi-pack-index write &&
cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
# Write a new pack that is unknown to the multi-pack-index.
BLOB1=$(echo blob1 | git hash-object -w --stdin) &&
BLOB2=$(echo blob2 | git hash-object -w --stdin) &&
cat >blobs <<-EOF &&
$BLOB1
$BLOB2
EOF
HASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&
HASH2=$(git pack-objects $objdir/pack/pack <blobs) &&
GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
test_cmp_bin $objdir/pack/multi-pack-index \
$objdir/pack/multi-pack-index.bak &&
test_path_is_file $objdir/pack/pack-$HASH2.pack &&
test_path_is_missing $objdir/pack/pack-$HASH1.pack
'
However, this _still_ fails on the "test_path_is_missing" line, so I'm not sure
how to make sure your logic is tested. I saw that 'git repack' was writing
"nothing new to pack" in the output, so I also tested adding a few commits and
trying to force it to repack reachable data, but I cannot seem to trigger it
to create a new pack that overrides only one pack that is not in the MIDX.
Likely, I just don't know how 'git rebase' works well enough to trigger this
behavior. But the test as-is is not testing what you want it to test.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions
From: SZEDER Gábor @ 2020-08-25 13:47 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <ea23ba5e269305b660a1722254e2a933c14e5b57.1598283480.git.gitgitgadget@gmail.com>
On Mon, Aug 24, 2020 at 03:38:00PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Originally, all of Git's subcommands were implemented in their own
> executable/script, using the naming scheme `git-<command-name>`. When
> more and more functionality was turned into built-in commands (i.e. the
> `git` executable could run them without spawning a separate process),
> for backwards-compatibility, we hard-link the `git` executable to
> `git-<built-in>` for every built-in.
>
> This backwards-compatibility was needed to support scripts that called
> the dashed form, even if we deprecated that a _long_ time ago.
>
> For that reason, we just introduced a Makefile knob to skip linking
> them. TO make sure that this keeps working, teach the CI
> (and PR) builds to skip generating those hard-links.
I'm afraid I don't understand this patch or the previous one (or
both?). So this new Makefile knob stops hard-linking the dashed
builtins _during 'make install'_, but it doesn't affect how Git is
built by the default target. And our CI jobs only build Git by the
default target, but don't run 'make install', so setting
SKIP_DASHED_BUILT_INS wouldn't have any affect anyway.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> ci/run-build-and-tests.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 6c27b886b8..1df9402c3b 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
> *) ln -s "$cache_dir/.prove" t/.prove;;
> esac
>
> -make
> +make SKIP_DASHED_BUILT_INS=YesPlease
Note that the CI jobs executed in containers (Linux32 and linux-musl)
don't use this 'ci/run-build-and-tests.sh' script, so they won't set
SKIP_DASHED_BUILT_INS. I suppose that's unintentional, because it
wasn't mentioned in the commit message.
> case "$jobname" in
> linux-gcc)
> make test
> --
> gitgitgadget
^ permalink raw reply
* Re: [PATCH 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
From: Kaartic Sivaraam @ 2020-08-25 14:33 UTC (permalink / raw)
To: Shourya Shukla
Cc: git, gitster, christian.couder, Johannes.Schindelin, peff,
liu.denton, Christian Couder
In-Reply-To: <20200825113020.71801-4-shouryashukla.oo@gmail.com>
On 25-08-2020 17:00, Shourya Shukla wrote:
> The 'grep' check in test 4 of t7421 resulted in the failure of t7421 on
> Windows due to a different error message
>
> error: cannot spawn git: No such file or directory
>
> instead of
>
> fatal: exec 'rev-parse': cd to 'my-subm' failed: No such file or directory
>
> Tighten up the check to compute '{src,dst}_abbrev' by guarding the
The change only affects `src_abbrev`. So, it's misleading to mention
`dst_abbrev` here.
> 'verify_submodule_committish()' call using `p->status !='D'`, so that
> the former isn't called in case of non-existent submodule directory,
> consequently, there is no such error message on any execution
> environment.
>
> Therefore, eliminate the 'grep' check in t7421. Instead, verify the
> absence of an error message by doing a 'test_must_be_empty' on the
> file containing the error.
>
> Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> builtin/submodule--helper.c | 7 ++++---
> t/t7421-submodule-summary-add.sh | 2 +-
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 93d0700891..f1951680f7 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1035,7 +1035,7 @@ static void print_submodule_summary(struct summary_cb *info, char *errmsg,
> static void generate_submodule_summary(struct summary_cb *info,
> struct module_cb *p)
> {
> - char *displaypath, *src_abbrev, *dst_abbrev;
> + char *displaypath, *src_abbrev = NULL, *dst_abbrev = NULL;
Unlike `src_abbrev`, I don't think we need to initilialize `dst_abbrev`
to NULL here as it would be assigned in all code paths.
> int missing_src = 0, missing_dst = 0;
> char *errmsg = NULL;
> int total_commits = -1;
> @@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info,
> }
>
> if (S_ISGITLINK(p->mod_src)) {
> - src_abbrev = verify_submodule_committish(p->sm_path,
> - oid_to_hex(&p->oid_src));
> + if (p->status != 'D')
> + src_abbrev = verify_submodule_committish(p->sm_path,
> + oid_to_hex(&p->oid_src));
> if (!src_abbrev) {
> missing_src = 1;
> /*
--
Sivaraam
^ permalink raw reply
* Re: [GSoC][PATCH 0/3] submodule: fixup to summary-v3
From: Kaartic Sivaraam @ 2020-08-25 14:38 UTC (permalink / raw)
To: Shourya Shukla
Cc: git, gitster, christian.couder, Johannes.Schindelin, peff,
liu.denton
In-Reply-To: <20200825113020.71801-1-shouryashukla.oo@gmail.com>
On 25-08-2020 17:00, Shourya Shukla wrote:
> Greetings,
>
> The v3 of 'git submodule summary' port to C is currently in 'next'
> branch of git/git. Recently, the patch recieved some comments from
> Peff, Dscho and Kaartic:
>
> 1. The definition of 'print_submodule_summary()' contained two
> unused parameters namely 'missing_src' and 'missing_dst'. Hence,
> I had to eliminate them as covered in the commit a22ffa950f
> a22ffa950f (submodule: eliminate unused parameters from
> print_submodule_summary(), 2020-08-21). Reported by Peff.
> Junio also advised to make the output in case of an unexpected
> file mode a bit more user friendly by outputting an octal instead
> of a decimal.
>
> 2. The function definitions of 'verify_submodule_committish()' and
> 'print_submodule_summary()' had wrong styling in terms of the
> asterisk placement. Hence it was fixed in 32934998ee (submodule:
> fix style in function definition, 2020-08-22). Reported by
> Kaartic.
>
> 2. The test script 't7421-submodule-summary-add.sh' failed in
> Windows due to failure of t7421.4. Precisely, the 'test_i18ngrep'
> check failed on Windows since the error message which was being
> grepped was different on Windows; it was designed to work on
> Linux. Therefore, we had to eliminate the grep check in t7421.4
> and replace it with a check to see if there is any error message
> or not using 'test_must_be_empty'. Also, to support this change,
> we had to make some small changes in 'print_submodule_summary()'
> function. The call to verify_submodule_committish()' had to be
> guarded using 'p->status !=D' so that it isn't called when the SM
> directory does not exist, therefore, the error message is not
> displayed. This resulted in 82e0956cd2 (t7421: eliminate 'grep'
> check in t7421.4 for mingw compatibility, 2020-08-22). Reported
> by Dscho.
>
While the cover letter is nice, it doesn't make much sense to refer to
patches that are part of the series using the commit hashes of their
"local" commits. It's more common to refer to them as using the position
of the patch such as [1/3] etc.
--
Sivaraam
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Taylor Blau @ 2020-08-25 14:41 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Taylor Blau, Jeff King, git, dstolee
In-Reply-To: <e7eb9fb6-f1ea-f932-efaa-7434ad809989@gmail.com>
On Tue, Aug 25, 2020 at 09:14:19AM -0400, Derrick Stolee wrote:
> On 8/24/2020 10:37 PM, Taylor Blau wrote:
> > On Mon, Aug 24, 2020 at 10:26:14PM -0400, Jeff King wrote:
> >> On Mon, Aug 24, 2020 at 10:01:04PM -0400, Taylor Blau wrote:
> >>
> >>> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'
> >>> learned to remove a multi-pack-index file if it added or removed a pack
> >>> from the object store.
> >>>
> >>> This mechanism is a little over-eager, since it is only necessary to
> >>> drop a MIDX if 'git repack' removes a pack that the MIDX references.
> >>> Adding a pack outside of the MIDX does not require invalidating the
> >>> MIDX, and likewise for removing a pack the MIDX does not know about.
> >>
> >> Does "git repack" ever remove just one pack? Obviously "git repack -ad"
> >> or "git repack -Ad" is going to pack everything and delete the old
> >> packs. So I think we'd want to remove a midx there.
> >>
> >> And "git repack -d" I think of as deleting only loose objects that we
> >> just packed. But I guess it could also remove a pack that has now been
> >> made redundant? That seems like a rare case in practice, but I suppose
> >> is possible.
> >
> > Yeah, the patch message makes this sound more likely than it actually
> > is, which I agree is very rare. I often write 'git repack' instead of
> > 'git pack-objects' to slurp up everything loose into a new pack without
> > having to list loose objects by name.
> >
> > That's the case that I really care about here: purely adding a new pack
> > should not invalidate the existing MIDX.
> >
> >> Not exactly related to your fix, but kind of the flip side of it: would
> >> we ever need to retain a midx that mentions some packs that still exist?
> >>
> >> E.g., imagine we have a midx that points to packs A and B, and
> >> git-repack deletes B. By your logic above, we need to remove the midx
> >> because now it points to objects in B which aren't accessible. But by
> >> deleting it, could we be deleting the only thing that mentions the
> >> objects in A?
> >>
> >> I _think_ the answer is "no", because we never went all-in on midx and
> >> allowed deleting the matching .idx files for contained packs. So we'd
> >> still have that A.idx, and we could just use the pack as normal. But
> >> it's an interesting corner case if we ever do go in that direction.
> >
> > Agreed. Maybe a (admittedly somewhat large) #leftoverbits.
> >
> >> If you'll let me muse a bit more on midx-lifetime issues (which I've
> >> never really thought about before just now):
> >>
> >> I'm also a little curious how bad it is to have a midx whose pack has
> >> gone away. I guess we'd answer queries for "yes, we have this object"
> >> even if we don't, which is bad. Though in practice we'd only delete
> >> those packs if we have their objects elsewhere. And the pack code is
> >> pretty good about retrying other copies of objects that can't be
> >> accessed. Alternatively, I wonder if the midx-loading code ought to
> >> check that all of the constituent packs are available.
> >>
> >> In that line of thinking, do we even need to delete midx files if one of
> >> their packs goes away? The reading side probably ought to be able to
> >> handle that gracefully.
> >
> > I think that this is probably the right direction, although I've only
> > spend time in the MIDX code over the past couple of weeks, so I can't
> > say with authority. It seems like it would be pretty annoying, though.
> > For example, code that cares about listing all objects in a MIDX would
> > have to check first whether the pack they're in still exists before
> > emitting them. On top of that, there are more corner cases when object X
> > exists in more than one pack, but some strict subset of those packs
> > containing X have gone away.
> >
> > I don't think that it couldn't be done, though.
> >
> >> And the more interesting case is when you repack everything with "-ad"
> >> or similar, at which point you shouldn't even need to look up what's in
> >> the midx to see if you deleted its packs. The point of your operation is
> >> to put it all-into-one, so you know the old midx should be discarded.
> >>
> >>> Teach 'git repack' to check for this by loading the MIDX, and checking
> >>> whether the to-be-removed pack is known to the MIDX. This requires a
> >>> slightly odd alternation to a test in t5319, which is explained with a
> >>> comment.
> >>
> >> My above musings aside, this seems like an obvious improvement.
> >>
> >>> diff --git a/builtin/repack.c b/builtin/repack.c
> >>> index 04c5ceaf7e..98fac03946 100644
> >>> --- a/builtin/repack.c
> >>> +++ b/builtin/repack.c
> >>> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,
> >>> static void remove_redundant_pack(const char *dir_name, const char *base_name)
> >>> {
> >>> struct strbuf buf = STRBUF_INIT;
> >>> - strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
> >>> + struct multi_pack_index *m = get_multi_pack_index(the_repository);
> >>> + strbuf_addf(&buf, "%s.pack", base_name);
> >>> + if (m && midx_contains_pack(m, buf.buf))
> >>> + clear_midx_file(the_repository);
> >>> + strbuf_insertf(&buf, 0, "%s/", dir_name);
> >>
> >> Makes sense. midx_contains_pack() is a binary search, so we'll spend
> >> O(n log n) effort deleting the packs (I wondered if this might be
> >> accidentally quadratic over the number of packs).
> >
> > Right. The MIDX stores packs in lexographic order, so checking them is
> > O(log n), which we do at most 'n' times.
> >
> >> And after we clear, "m" will be NULL, so we'll do it at most once. Which
> >> is why you can get rid of the manual "midx_cleared" flag from the
> >> preimage.
> >
> > Yep. I thought briefly about passing 'm' as a parameter, but then you
> > have to worry about a dangling reference to
> > 'the_repository->objects->multi_pack_index' after calling
> > 'clear_midx_file()', so it's easier to look it up each time.
>
> The discussion in this thread matches my understanding of the
> situation.
>
> >> So the patch looks good to me.
>
> The code in builtin/repack.c looks good for sure. I have a quick question
> about this new test:
>
> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> + git multi-pack-index write &&
> + cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> + test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> +
> + # Write a new pack that is unknown to the multi-pack-index.
> + git hash-object -w </dev/null >blob &&
> + git pack-objects $objdir/pack/pack <blob &&
> +
> + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> + test_cmp_bin $objdir/pack/multi-pack-index \
> + $objdir/pack/multi-pack-index.bak
> +'
> +
>
> You create an arbitrary blob, and then add it to a pack-file. Do we
> know that 'git repack' is definitely creating a new pack-file that makes
> our manually-created pack-file redundant?
>
> My suggestion is to have the test check itself:
>
> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> + git multi-pack-index write &&
> + cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> + test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> +
> + # Write a new pack that is unknown to the multi-pack-index.
> + git hash-object -w </dev/null >blob &&
> + HASH=$(git pack-objects $objdir/pack/pack <blob) &&
> +
> + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> + test_cmp_bin $objdir/pack/multi-pack-index \
> + $objdir/pack/multi-pack-index.bak &&
> + test_path_is_missing $objdir/pack/pack-$HASH.pack
> +'
> +
>
> This test fails for me, on the 'test_path_is_missing'. Likely, the
> blob is seen as already in a pack-file so is just pruned by 'git repack'
> instead. I thought that perhaps we need to add a new pack ourselves that
> overrides the small pack. Here is my attempt:
>
> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> git multi-pack-index write &&
> cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
>
> # Write a new pack that is unknown to the multi-pack-index.
> BLOB1=$(echo blob1 | git hash-object -w --stdin) &&
> BLOB2=$(echo blob2 | git hash-object -w --stdin) &&
> cat >blobs <<-EOF &&
> $BLOB1
> $BLOB2
> EOF
> HASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&
> HASH2=$(git pack-objects $objdir/pack/pack <blobs) &&
> GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> test_cmp_bin $objdir/pack/multi-pack-index \
> $objdir/pack/multi-pack-index.bak &&
> test_path_is_file $objdir/pack/pack-$HASH2.pack &&
> test_path_is_missing $objdir/pack/pack-$HASH1.pack
> '
>
> However, this _still_ fails on the "test_path_is_missing" line, so I'm not sure
> how to make sure your logic is tested. I saw that 'git repack' was writing
> "nothing new to pack" in the output, so I also tested adding a few commits and
> trying to force it to repack reachable data, but I cannot seem to trigger it
> to create a new pack that overrides only one pack that is not in the MIDX.
>
> Likely, I just don't know how 'git rebase' works well enough to trigger this
> behavior. But the test as-is is not testing what you want it to test.
I think this case might actually be impossible to tickle in a test. I
thought that 'git repack -d' looked for existing packs whose objects are
a subset of some new pack generated. But, it's much simpler than that:
'-d' by itself just looks for packs that were already on disk with the
same SHA-1 as a new pack, and it removes the old one.
Note that 'git repack' uses 'git pack-objects' internally to find
objects and generate a packfile. When calling 'git pack-objects', 'git
repack -d' passes '--all' and '--unpacked', which means that there is no
way we'd generate a new pack with the same SHA-1 as an existing pack
ordinarily.
So, I think this case is impossible, or at least astronomically
unlikely. What is more interesting (and untested) is that adding a _new_
pack doesn't cause us to invalidate the MIDX. Here's a patch that does
that:
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 16a1ad040e..620f2058d6 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -391,18 +391,27 @@ test_expect_success 'repack removes multi-pack-index when deleting packs' '
test_path_is_missing $objdir/pack/multi-pack-index
'
-test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
- git multi-pack-index write &&
- cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
- test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
-
- # Write a new pack that is unknown to the multi-pack-index.
- git hash-object -w </dev/null >blob &&
- git pack-objects $objdir/pack/pack <blob &&
-
- GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
- test_cmp_bin $objdir/pack/multi-pack-index \
- $objdir/pack/multi-pack-index.bak
+test_expect_success 'repack preserves multi-pack-index when creating packs' '
+ git init preserve &&
+ test_when_finished "rm -fr preserve" &&
+ (
+ cd preserve &&
+ midx=.git/objects/pack/multi-pack-index &&
+
+ test_commit "initial" &&
+ git repack -ad &&
+ git multi-pack-index write &&
+ ls .git/objects/pack | grep "\.pack$" >before &&
+
+ cp $midx $midx.bak &&
+
+ test_commit "another" &&
+ GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
+ ls .git/objects/pack | grep "\.pack$" >after &&
+
+ test_cmp_bin $midx.bak $midx &&
+ ! test_cmp before after
+ )
'
compare_results_with_midx "after repack"
What do you think about applying this on top and then calling it a day?
> Thanks,
> -Stolee
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Taylor Blau @ 2020-08-25 14:45 UTC (permalink / raw)
To: Son Luong Ngoc; +Cc: Taylor Blau, git, dstolee
In-Reply-To: <CB6B70D3-5FC6-43FE-8460-33F6CFC123E6@gmail.com>
On Tue, Aug 25, 2020 at 09:55:22AM +0200, Son Luong Ngoc wrote:
> Hi Taylor,
>
> Thanks for working on this.
>
> > On Aug 25, 2020, at 04:01, Taylor Blau <me@ttaylorr.com> wrote:
> >
> > In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'
> > learned to remove a multi-pack-index file if it added or removed a pack
> > from the object store.
> >
> > This mechanism is a little over-eager, since it is only necessary to
> > drop a MIDX if 'git repack' removes a pack that the MIDX references.
> > Adding a pack outside of the MIDX does not require invalidating the
> > MIDX, and likewise for removing a pack the MIDX does not know about.
>
> I wonder if its worth to trigger write_midx_file() to update the midx instead of
> just removing MIDX?
There's no reason that we couldn't do this, but I don't think that it's
a very good idea, especially if the new 'git maintenance' command will
be able to do something like this by itself.
I'm hesitant to add yet another option to 'git repack', which I have
always thought as a plumbing tool. That's important because callers
(like 'git maintenance' or user scripts) can 'git multi-pack-index write
...' after their 'git repack' to generate a new MIDX if they want one.
This becomes even trickier if 'git multi-pack-index write' were to gain
new options, at which point 'git repack' would *also* have to learn
those options to plumb them through.
Maybe there's a legitimate case that I'm overlooking, but I don't think
so. If there is, I'd be happy to come back to this, but this patch is
really just focused on fixing a bug.
> That is already the direction we are taking in the 'maintenance' patch series
> whenever the multi-pack-index file was deemed invalid.
>
> Or perhaps, we can check for 'core.multiPackIndex' value (which recently is
> 'true' by default) and determine whether we should remove the MIDX or rewrite
> it?
>
> >
> > Teach 'git repack' to check for this by loading the MIDX, and checking
> > whether the to-be-removed pack is known to the MIDX. This requires a
> > slightly odd alternation to a test in t5319, which is explained with a
> > comment.
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
Thanks for your feedback.
> Cheers,
> Son Luong.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v2] refs: remove lookup cache for reference-transaction hook
From: Jeff King @ 2020-08-25 15:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Taylor Blau
In-Reply-To: <c1cae6dd19ffe00e4456e4f96ad92277ceeced27.1598349284.git.ps@pks.im>
On Tue, Aug 25, 2020 at 12:35:24PM +0200, Patrick Steinhardt wrote:
> The only change compared to v1 is that I've addressed the unportable
> `branch-{1..1000}` syntax in favor of `test_seq`. I had to setup refs as
> part of the setup and change the ordering for "update-ref --stdin" from
> create/update/delete to update/delete/create, but I don't think that's
> too bad. At least timings didn't seem to really change because of that.
Another option instead of changing the order in the other tests is to do
another untimed setup step before the push test. I'm OK either way,
though.
> +test_perf "nonatomic push" '
> + git push ./target-repo.git $(test_seq 1000) &&
> + git push --delete ./target-repo.git $(test_seq 1000)
> '
This works as far as Git is concerned, but "seq 1000" output with NULs
is 3893 bytes. I wonder if some platforms might run into command-line
limits there. I guess we will see when Windows CI runs. :)
-Peff
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Derrick Stolee @ 2020-08-25 15:14 UTC (permalink / raw)
To: Taylor Blau; +Cc: Jeff King, git, dstolee
In-Reply-To: <20200825144146.GA7671@syl.lan>
On 8/25/2020 10:41 AM, Taylor Blau wrote:
> On Tue, Aug 25, 2020 at 09:14:19AM -0400, Derrick Stolee wrote:
>> The code in builtin/repack.c looks good for sure. I have a quick question
>> about this new test:
>>
>> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
>> + git multi-pack-index write &&
>> + cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
>> + test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
>> +
>> + # Write a new pack that is unknown to the multi-pack-index.
>> + git hash-object -w </dev/null >blob &&
>> + git pack-objects $objdir/pack/pack <blob &&
>> +
>> + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
>> + test_cmp_bin $objdir/pack/multi-pack-index \
>> + $objdir/pack/multi-pack-index.bak
>> +'
>> +
>>
>> You create an arbitrary blob, and then add it to a pack-file. Do we
>> know that 'git repack' is definitely creating a new pack-file that makes
>> our manually-created pack-file redundant?
>>
>> My suggestion is to have the test check itself:
>>
>> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
>> + git multi-pack-index write &&
>> + cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
>> + test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
>> +
>> + # Write a new pack that is unknown to the multi-pack-index.
>> + git hash-object -w </dev/null >blob &&
>> + HASH=$(git pack-objects $objdir/pack/pack <blob) &&
>> +
>> + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
>> + test_cmp_bin $objdir/pack/multi-pack-index \
>> + $objdir/pack/multi-pack-index.bak &&
>> + test_path_is_missing $objdir/pack/pack-$HASH.pack
>> +'
>> +
>>
>> This test fails for me, on the 'test_path_is_missing'. Likely, the
>> blob is seen as already in a pack-file so is just pruned by 'git repack'
>> instead. I thought that perhaps we need to add a new pack ourselves that
>> overrides the small pack. Here is my attempt:
>>
>> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
>> git multi-pack-index write &&
>> cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
>> test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
>>
>> # Write a new pack that is unknown to the multi-pack-index.
>> BLOB1=$(echo blob1 | git hash-object -w --stdin) &&
>> BLOB2=$(echo blob2 | git hash-object -w --stdin) &&
>> cat >blobs <<-EOF &&
>> $BLOB1
>> $BLOB2
>> EOF
>> HASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&
>> HASH2=$(git pack-objects $objdir/pack/pack <blobs) &&
>> GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
>> test_cmp_bin $objdir/pack/multi-pack-index \
>> $objdir/pack/multi-pack-index.bak &&
>> test_path_is_file $objdir/pack/pack-$HASH2.pack &&
>> test_path_is_missing $objdir/pack/pack-$HASH1.pack
>> '
>>
>> However, this _still_ fails on the "test_path_is_missing" line, so I'm not sure
>> how to make sure your logic is tested. I saw that 'git repack' was writing
>> "nothing new to pack" in the output, so I also tested adding a few commits and
>> trying to force it to repack reachable data, but I cannot seem to trigger it
>> to create a new pack that overrides only one pack that is not in the MIDX.
>>
>> Likely, I just don't know how 'git rebase' works well enough to trigger this
>> behavior. But the test as-is is not testing what you want it to test.
>
> I think this case might actually be impossible to tickle in a test. I
> thought that 'git repack -d' looked for existing packs whose objects are
> a subset of some new pack generated. But, it's much simpler than that:
> '-d' by itself just looks for packs that were already on disk with the
> same SHA-1 as a new pack, and it removes the old one.
If 'git repack' never calls remove_redundant_pack() unless we are doing
a "full repack", then we could simplify this logic:
static void remove_redundant_pack(const char *dir_name, const char *base_name)
{
struct strbuf buf = STRBUF_INIT;
- strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
+ struct multi_pack_index *m = get_multi_pack_index(the_repository);
+ strbuf_addf(&buf, "%s.pack", base_name);
+ if (m && midx_contains_pack(m, buf.buf))
+ clear_midx_file(the_repository);
+ strbuf_insertf(&buf, 0, "%s/", dir_name);
unlink_pack_path(buf.buf, 1);
strbuf_release(&buf);
}
to
static void remove_redundant_pack(const char *dir_name, const char *base_name)
{
struct strbuf buf = STRBUF_INIT;
strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
+ clear_midx_file(the_repository);
unlink_pack_path(buf.buf, 1);
strbuf_release(&buf);
}
and get the same results as we are showing in these tests. This does
move us incrementally to a better situation: don't delete the MIDX
if we aren't deleting pack files. But, I think we can get around it.
> Note that 'git repack' uses 'git pack-objects' internally to find
> objects and generate a packfile. When calling 'git pack-objects', 'git
> repack -d' passes '--all' and '--unpacked', which means that there is no
> way we'd generate a new pack with the same SHA-1 as an existing pack
> ordinarily.
>
> So, I think this case is impossible, or at least astronomically
> unlikely. What is more interesting (and untested) is that adding a _new_
> pack doesn't cause us to invalidate the MIDX. Here's a patch that does
> that:
>
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 16a1ad040e..620f2058d6 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -391,18 +391,27 @@ test_expect_success 'repack removes multi-pack-index when deleting packs' '
> test_path_is_missing $objdir/pack/multi-pack-index
> '
>
> -test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> - git multi-pack-index write &&
> - cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> - test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> -
> - # Write a new pack that is unknown to the multi-pack-index.
> - git hash-object -w </dev/null >blob &&
> - git pack-objects $objdir/pack/pack <blob &&
> -
> - GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> - test_cmp_bin $objdir/pack/multi-pack-index \
> - $objdir/pack/multi-pack-index.bak
> +test_expect_success 'repack preserves multi-pack-index when creating packs' '
> + git init preserve &&
> + test_when_finished "rm -fr preserve" &&
> + (
> + cd preserve &&
> + midx=.git/objects/pack/multi-pack-index &&
> +
> + test_commit "initial" &&
> + git repack -ad &&
> + git multi-pack-index write &&
> + ls .git/objects/pack | grep "\.pack$" >before &&
> +
> + cp $midx $midx.bak &&
> +
> + test_commit "another" &&
> + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> + ls .git/objects/pack | grep "\.pack$" >after &&
> +
> + test_cmp_bin $midx.bak $midx &&
> + ! test_cmp before after
> + )
> '
After looking at the callers to remote_redundant_pack() I noticed that it is only
called after inspecting the "names" struct, which contains the names of the packs
to group into a new pack-file. We can use a .keep file to preserve the pack-file(s) in
the MIDX but also to ensure multiple pack-files outside of the MIDX are repacked and
deleted. While this is very unlikely in the wild, it is definitely possible.
test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
git init preserve &&
test_when_finished "rm -fr preserve" &&
(
cd preserve &&
midx=.git/objects/pack/multi-pack-index &&
test_commit 1 &&
HASH1=$(git pack-objects --all .git/objects/pack/pack) &&
touch .git/objects/pack/pack-$HASH1.keep &&
cat >pack-input <<-\EOF &&
HEAD
^HEAD~1
EOF
test_commit 2 &&
HASH2=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
touch .git/objects/pack/pack-$HASH2.keep &&
git multi-pack-index write &&
cp $midx $midx.bak &&
test_commit 3 &&
HASH3=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
test_commit 4 &&
HASH4=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -ad &&
test_path_is_file .git/objects/pack/pack-$HASH1.pack &&
test_path_is_file .git/objects/pack/pack-$HASH2.pack &&
test_path_is_missing .git/objects/pack/pack-$HASH3.pack &&
test_path_is_missing .git/objects/pack/pack-$HASH4.pack
)
'
I believe this checks your condition properly enough.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH v2 3/3] ci: stop linking built-ins to the dashed versions
From: Junio C Hamano @ 2020-08-25 15:42 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
In-Reply-To: <20200825134714.GC25052@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> I'm afraid I don't understand this patch or the previous one (or
> both?). So this new Makefile knob stops hard-linking the dashed
> builtins _during 'make install'_, but it doesn't affect how Git is
> built by the default target. And our CI jobs only build Git by the
> default target, but don't run 'make install', so setting
> SKIP_DASHED_BUILT_INS wouldn't have any affect anyway.
Very very true. Let's drop 3/3 if it is not testing anything new.
I do understand the concern 2/3 wants to address, and it would be a
real one to you especially if you come from Windows. People on the
platform wouldn't be able to use shell scripts written in 12 years
ago or written with the promise we made to our users 12 years ago,
and unlike hardlink-capable platforms it incurs real cost to install
these individual binaries on disk.
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Taylor Blau @ 2020-08-25 15:42 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Taylor Blau, Jeff King, git, dstolee
In-Reply-To: <6a34d7ee-8c6b-8c6c-93bd-0013dccccafb@gmail.com>
On Tue, Aug 25, 2020 at 11:14:41AM -0400, Derrick Stolee wrote:
> On 8/25/2020 10:41 AM, Taylor Blau wrote:
> > On Tue, Aug 25, 2020 at 09:14:19AM -0400, Derrick Stolee wrote:
> >> The code in builtin/repack.c looks good for sure. I have a quick question
> >> about this new test:
> >>
> >> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> >> + git multi-pack-index write &&
> >> + cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> >> + test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> >> +
> >> + # Write a new pack that is unknown to the multi-pack-index.
> >> + git hash-object -w </dev/null >blob &&
> >> + git pack-objects $objdir/pack/pack <blob &&
> >> +
> >> + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> >> + test_cmp_bin $objdir/pack/multi-pack-index \
> >> + $objdir/pack/multi-pack-index.bak
> >> +'
> >> +
> >>
> >> You create an arbitrary blob, and then add it to a pack-file. Do we
> >> know that 'git repack' is definitely creating a new pack-file that makes
> >> our manually-created pack-file redundant?
> >>
> >> My suggestion is to have the test check itself:
> >>
> >> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> >> + git multi-pack-index write &&
> >> + cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> >> + test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> >> +
> >> + # Write a new pack that is unknown to the multi-pack-index.
> >> + git hash-object -w </dev/null >blob &&
> >> + HASH=$(git pack-objects $objdir/pack/pack <blob) &&
> >> +
> >> + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> >> + test_cmp_bin $objdir/pack/multi-pack-index \
> >> + $objdir/pack/multi-pack-index.bak &&
> >> + test_path_is_missing $objdir/pack/pack-$HASH.pack
> >> +'
> >> +
> >>
> >> This test fails for me, on the 'test_path_is_missing'. Likely, the
> >> blob is seen as already in a pack-file so is just pruned by 'git repack'
> >> instead. I thought that perhaps we need to add a new pack ourselves that
> >> overrides the small pack. Here is my attempt:
> >>
> >> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> >> git multi-pack-index write &&
> >> cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> >> test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> >>
> >> # Write a new pack that is unknown to the multi-pack-index.
> >> BLOB1=$(echo blob1 | git hash-object -w --stdin) &&
> >> BLOB2=$(echo blob2 | git hash-object -w --stdin) &&
> >> cat >blobs <<-EOF &&
> >> $BLOB1
> >> $BLOB2
> >> EOF
> >> HASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&
> >> HASH2=$(git pack-objects $objdir/pack/pack <blobs) &&
> >> GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> >> test_cmp_bin $objdir/pack/multi-pack-index \
> >> $objdir/pack/multi-pack-index.bak &&
> >> test_path_is_file $objdir/pack/pack-$HASH2.pack &&
> >> test_path_is_missing $objdir/pack/pack-$HASH1.pack
> >> '
> >>
> >> However, this _still_ fails on the "test_path_is_missing" line, so I'm not sure
> >> how to make sure your logic is tested. I saw that 'git repack' was writing
> >> "nothing new to pack" in the output, so I also tested adding a few commits and
> >> trying to force it to repack reachable data, but I cannot seem to trigger it
> >> to create a new pack that overrides only one pack that is not in the MIDX.
> >>
> >> Likely, I just don't know how 'git rebase' works well enough to trigger this
> >> behavior. But the test as-is is not testing what you want it to test.
> >
> > I think this case might actually be impossible to tickle in a test. I
> > thought that 'git repack -d' looked for existing packs whose objects are
> > a subset of some new pack generated. But, it's much simpler than that:
> > '-d' by itself just looks for packs that were already on disk with the
> > same SHA-1 as a new pack, and it removes the old one.
>
> If 'git repack' never calls remove_redundant_pack() unless we are doing
> a "full repack", then we could simplify this logic:
>
> static void remove_redundant_pack(const char *dir_name, const char *base_name)
> {
> struct strbuf buf = STRBUF_INIT;
> - strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
> + struct multi_pack_index *m = get_multi_pack_index(the_repository);
> + strbuf_addf(&buf, "%s.pack", base_name);
> + if (m && midx_contains_pack(m, buf.buf))
> + clear_midx_file(the_repository);
> + strbuf_insertf(&buf, 0, "%s/", dir_name);
> unlink_pack_path(buf.buf, 1);
> strbuf_release(&buf);
> }
>
> to
>
> static void remove_redundant_pack(const char *dir_name, const char *base_name)
> {
> struct strbuf buf = STRBUF_INIT;
> strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
> + clear_midx_file(the_repository);
> unlink_pack_path(buf.buf, 1);
> strbuf_release(&buf);
> }
>
> and get the same results as we are showing in these tests. This does
> move us incrementally to a better situation: don't delete the MIDX
> if we aren't deleting pack files. But, I think we can get around it.
Makes sense, but reading your whole email we are better off leaving this
as-is and changing the test to exercise it more often.
> > Note that 'git repack' uses 'git pack-objects' internally to find
> > objects and generate a packfile. When calling 'git pack-objects', 'git
> > repack -d' passes '--all' and '--unpacked', which means that there is no
> > way we'd generate a new pack with the same SHA-1 as an existing pack
> > ordinarily.
> >
> > So, I think this case is impossible, or at least astronomically
> > unlikely. What is more interesting (and untested) is that adding a _new_
> > pack doesn't cause us to invalidate the MIDX. Here's a patch that does
> > that:
> >
> > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> > index 16a1ad040e..620f2058d6 100755
> > --- a/t/t5319-multi-pack-index.sh
> > +++ b/t/t5319-multi-pack-index.sh
> > @@ -391,18 +391,27 @@ test_expect_success 'repack removes multi-pack-index when deleting packs' '
> > test_path_is_missing $objdir/pack/multi-pack-index
> > '
> >
> > -test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> > - git multi-pack-index write &&
> > - cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
> > - test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
> > -
> > - # Write a new pack that is unknown to the multi-pack-index.
> > - git hash-object -w </dev/null >blob &&
> > - git pack-objects $objdir/pack/pack <blob &&
> > -
> > - GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> > - test_cmp_bin $objdir/pack/multi-pack-index \
> > - $objdir/pack/multi-pack-index.bak
> > +test_expect_success 'repack preserves multi-pack-index when creating packs' '
> > + git init preserve &&
> > + test_when_finished "rm -fr preserve" &&
> > + (
> > + cd preserve &&
> > + midx=.git/objects/pack/multi-pack-index &&
> > +
> > + test_commit "initial" &&
> > + git repack -ad &&
> > + git multi-pack-index write &&
> > + ls .git/objects/pack | grep "\.pack$" >before &&
> > +
> > + cp $midx $midx.bak &&
> > +
> > + test_commit "another" &&
> > + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
> > + ls .git/objects/pack | grep "\.pack$" >after &&
> > +
> > + test_cmp_bin $midx.bak $midx &&
> > + ! test_cmp before after
> > + )
> > '
>
> After looking at the callers to remote_redundant_pack() I noticed that it is only
> called after inspecting the "names" struct, which contains the names of the packs
> to group into a new pack-file. We can use a .keep file to preserve the pack-file(s) in
> the MIDX but also to ensure multiple pack-files outside of the MIDX are repacked and
> deleted. While this is very unlikely in the wild, it is definitely possible.
Great idea.
> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> git init preserve &&
> test_when_finished "rm -fr preserve" &&
> (
> cd preserve &&
> midx=.git/objects/pack/multi-pack-index &&
>
> test_commit 1 &&
> HASH1=$(git pack-objects --all .git/objects/pack/pack) &&
> touch .git/objects/pack/pack-$HASH1.keep &&
>
> cat >pack-input <<-\EOF &&
Escaping the heredoc shouldn't be necessary, so this can be written
instead as '<<-EOF'.
> HEAD
> ^HEAD~1
> EOF
>
> test_commit 2 &&
> HASH2=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
> touch .git/objects/pack/pack-$HASH2.keep &&
>
> git multi-pack-index write &&
> cp $midx $midx.bak &&
>
> test_commit 3 &&
> HASH3=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
>
> test_commit 4 &&
> HASH4=$(git pack-objects --revs .git/objects/pack/pack <pack-input) &&
>
> GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -ad &&
> test_path_is_file .git/objects/pack/pack-$HASH1.pack &&
> test_path_is_file .git/objects/pack/pack-$HASH2.pack &&
> test_path_is_missing .git/objects/pack/pack-$HASH3.pack &&
> test_path_is_missing .git/objects/pack/pack-$HASH4.pack
...and we should check that 'test_cmp $midx.bak $midx' is clean, i.e.,
that we didn't touch the MIDX.
> )
> '
>
> I believe this checks your condition properly enough.
Otherwise, I think that this test looks great. Thanks for suggesting
it. I'll send a new patch now...
> Thanks,
> -Stolee
Thanks,
Taylor
^ permalink raw reply
* Re: pw/add-p-allowed-options-fix, was Re: What's cooking in git.git (Aug 2020, #06; Mon, 24)
From: Junio C Hamano @ 2020-08-25 15:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <nycvar.QRO.7.76.6.2008251056290.56@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Mon, 24 Aug 2020, Junio C Hamano wrote:
>
>> * pw/add-p-allowed-options-fix (2020-08-17) 2 commits
>> - add -p: fix checking of user input
>> - add -p: use ALLOC_GROW_BY instead of ALLOW_GROW
>>
>> "git add -p" update.
>>
>> Will merge to 'next'.
>
> I thought the consensus was to untangle the refactoring in the second
> patch by first fixing the `e` case, and _then_ refactoring?
Not really. My take on it is that everybody would agree that the
split would be to "fix 'e' without introducing the code structure
change" then to "change the code structure to make the bug
impossible", _IF_ we were to split the "fix" part into two. I do
not think anybody agreed to anything beyond that, and I am leaning
toward keeping the "let's change the code structure to make such a
bug impossible to introduce---oops, as a side effect we already
fixed the 'e' bug", which, as you say, is more than good enough.
> With Phillip being offline for a couple days, however, I think we can just
> go forward with the patches as-are. They are in "good enough" a shape.
Thanks.
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Junio C Hamano @ 2020-08-25 15:58 UTC (permalink / raw)
To: Jeff King; +Cc: Taylor Blau, git, dstolee
In-Reply-To: <20200825022614.GA1391422@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Does "git repack" ever remove just one pack? Obviously "git repack -ad"
> or "git repack -Ad" is going to pack everything and delete the old
> packs. So I think we'd want to remove a midx there.
AFAIK, the pack-redundant subcommand is used by nobody in-tree, so
nobody is doing "there are three packfiles, but all the objects in
one of them are contained in the other two, so let's remove that
redundant one".
> And "git repack -d" I think of as deleting only loose objects that we
> just packed. But I guess it could also remove a pack that has now been
> made redundant? That seems like a rare case in practice, but I suppose
> is possible.
Meaning it can become reality? Yes. Or it already can happen? I
doubt it.
> E.g., imagine we have a midx that points to packs A and B, and
> git-repack deletes B. By your logic above, we need to remove the midx
> because now it points to objects in B which aren't accessible. But by
> deleting it, could we be deleting the only thing that mentions the
> objects in A?
>
> I _think_ the answer is "no", because we never went all-in on midx and
> allowed deleting the matching .idx files for contained packs.
Yeah, it has been my assumption that that part of the design would
never change.
> I'm also a little curious how bad it is to have a midx whose pack has
> gone away. I guess we'd answer queries for "yes, we have this object"
> even if we don't, which is bad. Though in practice we'd only delete
> those packs if we have their objects elsewhere.
Hmph, object O used to be in pack A and pack B, midx points at the
copy in pack B but we deleted it because the pack is redundant.
Now, midx says O exists, but midx cannot be used to retrieve O. We
need to ask A.idx about O to locate it.
That sounds brittle. I am not sure our error fallback is all that
patient.
> And the pack code is
> pretty good about retrying other copies of objects that can't be
> accessed. Alternatively, I wonder if the midx-loading code ought to
> check that all of the constituent packs are available.
>
> In that line of thinking, do we even need to delete midx files if one of
> their packs goes away? The reading side probably ought to be able to
> handle that gracefully.
But at that point, is there even a point to have that midx file that
knows about objects (so it can answer has_object()? correctly and
quickly) but does not know the correct location of half of the objects?
Instead of going directly to A.idx to locate O, we need to go to midx
to learn the location of O in B (which no longer exists), and then
fall back to it, that is a pure overhead.
> And the more interesting case is when you repack everything with "-ad"
> or similar, at which point you shouldn't even need to look up what's in
> the midx to see if you deleted its packs. The point of your operation is
> to put it all-into-one, so you know the old midx should be discarded.
Old one, yes. Do we need to have the new one in that case?
>> Teach 'git repack' to check for this by loading the MIDX, and checking
>> whether the to-be-removed pack is known to the MIDX. This requires a
>> slightly odd alternation to a test in t5319, which is explained with a
>> comment.
>
> My above musings aside, this seems like an obvious improvement.
Yes.
>> diff --git a/builtin/repack.c b/builtin/repack.c
>> index 04c5ceaf7e..98fac03946 100644
>> --- a/builtin/repack.c
>> +++ b/builtin/repack.c
>> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,
>> static void remove_redundant_pack(const char *dir_name, const char *base_name)
>> {
>> struct strbuf buf = STRBUF_INIT;
>> - strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
>> + struct multi_pack_index *m = get_multi_pack_index(the_repository);
>> + strbuf_addf(&buf, "%s.pack", base_name);
>> + if (m && midx_contains_pack(m, buf.buf))
>> + clear_midx_file(the_repository);
>> + strbuf_insertf(&buf, 0, "%s/", dir_name);
>
> Makes sense. midx_contains_pack() is a binary search, so we'll spend
> O(n log n) effort deleting the packs (I wondered if this might be
> accidentally quadratic over the number of packs).
>
> And after we clear, "m" will be NULL, so we'll do it at most once. Which
> is why you can get rid of the manual "midx_cleared" flag from the
> preimage.
>
> So the patch looks good to me.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/3] Optionally skip linking/copying the built-ins
From: Junio C Hamano @ 2020-08-25 16:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin, git
In-Reply-To: <nycvar.QRO.7.76.6.2008251006210.56@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> But if all you're interested in is the part before the actual email
> address, isn't "Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com>" almost identical to "Johannes Schindelin
> <gitgitgadget@gmail.com>"?
Here is what I often see in my MUA.
O [ 75: Johannes Schindelin ] Re: [PATCH v2 3/3] ci: stop linking bu...
OA [ 20: Johannes Schindelin ] pw/add-p-allowed-options-fix, was Re: ...
O [ 106: Johannes Schindelin via] [PATCH] git-gui: accommodate for inten...
O [ 66: Johannes Schindelin via] [PATCH] mingw: improve performance of ...
The "via ..." part may change its length depending on how long the
real real name is, but it is irritating and more importantly conveys
harmful information, as I am trying to be as neutral as I can when
reviewing patches no matteer how they are delivered.
^ permalink raw reply
* [PATCH v2] builtin/repack.c: invalidate MIDX only when necessary
From: Taylor Blau @ 2020-08-25 16:04 UTC (permalink / raw)
To: git; +Cc: dstolee, peff, sluongng
In-Reply-To: <20200825144515.GB7671@syl.lan>
In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'
learned to remove a multi-pack-index file if it added or removed a pack
from the object store.
This mechanism is a little over-eager, since it is only necessary to
drop a MIDX if 'git repack' removes a pack that the MIDX references.
Adding a pack outside of the MIDX does not require invalidating the
MIDX, and likewise for removing a pack the MIDX does not know about.
Teach 'git repack' to check for this by loading the MIDX, and checking
whether the to-be-removed pack is known to the MIDX. This requires a
slightly odd alternation to a test in t5319, which is explained with a
comment. A new test is added to show that the MIDX is left alone when
both packs known to it are marked as .keep, but two packs unknown to it
are removed and combined into one new pack.
Helped-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Range-diff against v1:
1: ef9186a8df ! 1: 87a3b7a5a2 builtin/repack.c: invalidate MIDX only when necessary
@@ Commit message
Teach 'git repack' to check for this by loading the MIDX, and checking
whether the to-be-removed pack is known to the MIDX. This requires a
slightly odd alternation to a test in t5319, which is explained with a
- comment.
+ comment. A new test is added to show that the MIDX is left alone when
+ both packs known to it are marked as .keep, but two packs unknown to it
+ are removed and combined into one new pack.
+ Helped-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## builtin/repack.c ##
@@ t/t5319-multi-pack-index.sh: test_expect_success 'repack with the --no-progress
test_path_is_missing $objdir/pack/multi-pack-index
'
-+test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
-+ git multi-pack-index write &&
-+ cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&
-+ test_when_finished "rm -f $objdir/pack/multi-pack-index.bak" &&
-+
-+ # Write a new pack that is unknown to the multi-pack-index.
-+ git hash-object -w </dev/null >blob &&
-+ git pack-objects $objdir/pack/pack <blob &&
-+
-+ GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&
-+ test_cmp_bin $objdir/pack/multi-pack-index \
-+ $objdir/pack/multi-pack-index.bak
++test_expect_success 'repack preserves multi-pack-index when creating packs' '
++ git init preserve &&
++ test_when_finished "rm -fr preserve" &&
++ (
++ cd preserve &&
++ packdir=.git/objects/pack &&
++ midx=$packdir/multi-pack-index &&
++
++ test_commit 1 &&
++ pack1=$(git pack-objects --all $packdir/pack) &&
++ touch $packdir/pack-$pack1.keep &&
++ test_commit 2 &&
++ pack2=$(git pack-objects --revs $packdir/pack) &&
++ touch $packdir/pack-$pack2.keep &&
++
++ git multi-pack-index write &&
++ cp $midx $midx.bak &&
++
++ cat >pack-input <<-EOF &&
++ HEAD
++ ^HEAD~1
++ EOF
++ test_commit 3 &&
++ pack3=$(git pack-objects --revs $packdir/pack <pack-input) &&
++ test_commit 4 &&
++ pack4=$(git pack-objects --revs $packdir/pack <pack-input) &&
++
++ GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -ad &&
++ ls -la $packdir &&
++ test_path_is_file $packdir/pack-$pack1.pack &&
++ test_path_is_file $packdir/pack-$pack2.pack &&
++ test_path_is_missing $packdir/pack-$pack3.pack &&
++ test_path_is_missing $packdir/pack-$pack4.pack &&
++ test_cmp_bin $midx.bak $midx
++ )
+'
+
compare_results_with_midx "after repack"
builtin/repack.c | 12 +++++-----
t/t5319-multi-pack-index.sh | 44 +++++++++++++++++++++++++++++++++++--
2 files changed, 47 insertions(+), 9 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 04c5ceaf7e..98fac03946 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,
static void remove_redundant_pack(const char *dir_name, const char *base_name)
{
struct strbuf buf = STRBUF_INIT;
- strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
+ struct multi_pack_index *m = get_multi_pack_index(the_repository);
+ strbuf_addf(&buf, "%s.pack", base_name);
+ if (m && midx_contains_pack(m, buf.buf))
+ clear_midx_file(the_repository);
+ strbuf_insertf(&buf, 0, "%s/", dir_name);
unlink_pack_path(buf.buf, 1);
strbuf_release(&buf);
}
@@ -286,7 +290,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
int keep_unreachable = 0;
struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
int no_update_server_info = 0;
- int midx_cleared = 0;
struct pack_objects_args po_args = {NULL};
struct option builtin_repack_options[] = {
@@ -439,11 +442,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
char *fname, *fname_old;
- if (!midx_cleared) {
- clear_midx_file(the_repository);
- midx_cleared = 1;
- }
-
fname = mkpathdup("%s/pack-%s%s", packdir,
item->string, exts[ext].name);
if (!file_exists(fname)) {
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 43b1b5b2af..f340b376bc 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -382,12 +382,52 @@ test_expect_success 'repack with the --no-progress option' '
test_line_count = 0 err
'
-test_expect_success 'repack removes multi-pack-index' '
+test_expect_success 'repack removes multi-pack-index when deleting packs' '
test_path_is_file $objdir/pack/multi-pack-index &&
- GIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&
+ # Set GIT_TEST_MULTI_PACK_INDEX to 0 to avoid writing a new
+ # multi-pack-index after repacking, but set "core.multiPackIndex" to
+ # true so that "git repack" can read the existing MIDX.
+ GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -adf &&
test_path_is_missing $objdir/pack/multi-pack-index
'
+test_expect_success 'repack preserves multi-pack-index when creating packs' '
+ git init preserve &&
+ test_when_finished "rm -fr preserve" &&
+ (
+ cd preserve &&
+ packdir=.git/objects/pack &&
+ midx=$packdir/multi-pack-index &&
+
+ test_commit 1 &&
+ pack1=$(git pack-objects --all $packdir/pack) &&
+ touch $packdir/pack-$pack1.keep &&
+ test_commit 2 &&
+ pack2=$(git pack-objects --revs $packdir/pack) &&
+ touch $packdir/pack-$pack2.keep &&
+
+ git multi-pack-index write &&
+ cp $midx $midx.bak &&
+
+ cat >pack-input <<-EOF &&
+ HEAD
+ ^HEAD~1
+ EOF
+ test_commit 3 &&
+ pack3=$(git pack-objects --revs $packdir/pack <pack-input) &&
+ test_commit 4 &&
+ pack4=$(git pack-objects --revs $packdir/pack <pack-input) &&
+
+ GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -ad &&
+ ls -la $packdir &&
+ test_path_is_file $packdir/pack-$pack1.pack &&
+ test_path_is_file $packdir/pack-$pack2.pack &&
+ test_path_is_missing $packdir/pack-$pack3.pack &&
+ test_path_is_missing $packdir/pack-$pack4.pack &&
+ test_cmp_bin $midx.bak $midx
+ )
+'
+
compare_results_with_midx "after repack"
test_expect_success 'multi-pack-index and pack-bitmap' '
--
2.28.0.338.g87a3b7a5a2
^ permalink raw reply related
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Taylor Blau @ 2020-08-25 16:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, dstolee
In-Reply-To: <xmqqtuwq1zux.fsf@gitster.c.googlers.com>
On Tue, Aug 25, 2020 at 08:58:46AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Does "git repack" ever remove just one pack? Obviously "git repack -ad"
> > or "git repack -Ad" is going to pack everything and delete the old
> > packs. So I think we'd want to remove a midx there.
>
> AFAIK, the pack-redundant subcommand is used by nobody in-tree, so
> nobody is doing "there are three packfiles, but all the objects in
> one of them are contained in the other two, so let's remove that
> redundant one".
Not AFAICT either.
> > And "git repack -d" I think of as deleting only loose objects that we
> > just packed. But I guess it could also remove a pack that has now been
> > made redundant? That seems like a rare case in practice, but I suppose
> > is possible.
>
> Meaning it can become reality? Yes. Or it already can happen? I
> doubt it.
>
> > E.g., imagine we have a midx that points to packs A and B, and
> > git-repack deletes B. By your logic above, we need to remove the midx
> > because now it points to objects in B which aren't accessible. But by
> > deleting it, could we be deleting the only thing that mentions the
> > objects in A?
> >
> > I _think_ the answer is "no", because we never went all-in on midx and
> > allowed deleting the matching .idx files for contained packs.
>
> Yeah, it has been my assumption that that part of the design would
> never change.
>
> > I'm also a little curious how bad it is to have a midx whose pack has
> > gone away. I guess we'd answer queries for "yes, we have this object"
> > even if we don't, which is bad. Though in practice we'd only delete
> > those packs if we have their objects elsewhere.
>
> Hmph, object O used to be in pack A and pack B, midx points at the
> copy in pack B but we deleted it because the pack is redundant.
> Now, midx says O exists, but midx cannot be used to retrieve O. We
> need to ask A.idx about O to locate it.
>
> That sounds brittle. I am not sure our error fallback is all that
> patient.
Me either. Like I said, I think that all of this is possible at least in
theory, but in practice it may be somewhat annoying in cases like these.
>
> > And the pack code is
> > pretty good about retrying other copies of objects that can't be
> > accessed. Alternatively, I wonder if the midx-loading code ought to
> > check that all of the constituent packs are available.
> >
> > In that line of thinking, do we even need to delete midx files if one of
> > their packs goes away? The reading side probably ought to be able to
> > handle that gracefully.
>
> But at that point, is there even a point to have that midx file that
> knows about objects (so it can answer has_object()? correctly and
> quickly) but does not know the correct location of half of the objects?
> Instead of going directly to A.idx to locate O, we need to go to midx
> to learn the location of O in B (which no longer exists), and then
> fall back to it, that is a pure overhead.
Well put.
> > And the more interesting case is when you repack everything with "-ad"
> > or similar, at which point you shouldn't even need to look up what's in
> > the midx to see if you deleted its packs. The point of your operation is
> > to put it all-into-one, so you know the old midx should be discarded.
>
> Old one, yes. Do we need to have the new one in that case?
>
> >> Teach 'git repack' to check for this by loading the MIDX, and checking
> >> whether the to-be-removed pack is known to the MIDX. This requires a
> >> slightly odd alternation to a test in t5319, which is explained with a
> >> comment.
> >
> > My above musings aside, this seems like an obvious improvement.
>
> Yes.
>
> >> diff --git a/builtin/repack.c b/builtin/repack.c
> >> index 04c5ceaf7e..98fac03946 100644
> >> --- a/builtin/repack.c
> >> +++ b/builtin/repack.c
> >> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,
> >> static void remove_redundant_pack(const char *dir_name, const char *base_name)
> >> {
> >> struct strbuf buf = STRBUF_INIT;
> >> - strbuf_addf(&buf, "%s/%s.pack", dir_name, base_name);
> >> + struct multi_pack_index *m = get_multi_pack_index(the_repository);
> >> + strbuf_addf(&buf, "%s.pack", base_name);
> >> + if (m && midx_contains_pack(m, buf.buf))
> >> + clear_midx_file(the_repository);
> >> + strbuf_insertf(&buf, 0, "%s/", dir_name);
> >
> > Makes sense. midx_contains_pack() is a binary search, so we'll spend
> > O(n log n) effort deleting the packs (I wondered if this might be
> > accidentally quadratic over the number of packs).
> >
> > And after we clear, "m" will be NULL, so we'll do it at most once. Which
> > is why you can get rid of the manual "midx_cleared" flag from the
> > preimage.
> >
> > So the patch looks good to me.
>
> Thanks.
Thanks, both. If you're ready, let's use the version in [1] for
queueing.
Thanks,
Taylor
[1]: https://lore.kernel.org/git/87a3b7a5a2f091e2a23c163a7d86effbbbedfa3a.1598371475.git.me@ttaylorr.com/
^ permalink raw reply
* Re: [PATCH 3/3] t7421: eliminate 'grep' check in t7421.4 for mingw compatibility
From: Junio C Hamano @ 2020-08-25 16:10 UTC (permalink / raw)
To: Kaartic Sivaraam
Cc: Shourya Shukla, git, christian.couder, Johannes.Schindelin, peff,
liu.denton, Christian Couder
In-Reply-To: <2a1ea501-4974-4d74-fe3c-d173bbe76855@gmail.com>
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
>> @@ -1061,8 +1061,9 @@ static void generate_submodule_summary(struct summary_cb *info,
>> }
>>
>> if (S_ISGITLINK(p->mod_src)) {
>> - src_abbrev = verify_submodule_committish(p->sm_path,
>> - oid_to_hex(&p->oid_src));
>> + if (p->status != 'D')
>> + src_abbrev = verify_submodule_committish(p->sm_path,
>> + oid_to_hex(&p->oid_src));
>> if (!src_abbrev) {
>> missing_src = 1;
>> /*
Interesting. There is a mirroring if-else cascade that begins with
"if (S_ISGITLINK(p->mod_dst))" immediately after the if-else cascade
started here, and in there, the same verify_submodule_committish()
is called for oid_dst unconditionally. Should the asymmetry bother
readers of the code, or is the source side somehow special and needs
extra care?
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Derrick Stolee @ 2020-08-25 16:18 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Taylor Blau, git, dstolee
In-Reply-To: <xmqqtuwq1zux.fsf@gitster.c.googlers.com>
On 8/25/2020 11:58 AM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> Does "git repack" ever remove just one pack? Obviously "git repack -ad"
>> or "git repack -Ad" is going to pack everything and delete the old
>> packs. So I think we'd want to remove a midx there.
>
> AFAIK, the pack-redundant subcommand is used by nobody in-tree, so
> nobody is doing "there are three packfiles, but all the objects in
> one of them are contained in the other two, so let's remove that
> redundant one".
>
>> And "git repack -d" I think of as deleting only loose objects that we
>> just packed. But I guess it could also remove a pack that has now been
>> made redundant? That seems like a rare case in practice, but I suppose
>> is possible.
>
> Meaning it can become reality? Yes. Or it already can happen? I
> doubt it.
>
>> E.g., imagine we have a midx that points to packs A and B, and
>> git-repack deletes B. By your logic above, we need to remove the midx
>> because now it points to objects in B which aren't accessible. But by
>> deleting it, could we be deleting the only thing that mentions the
>> objects in A?
>>
>> I _think_ the answer is "no", because we never went all-in on midx and
>> allowed deleting the matching .idx files for contained packs.
>
> Yeah, it has been my assumption that that part of the design would
> never change.
Yes, we should intend to keep the .idx files around forever even when
using the multi-pack-index. The duplicated data overhead is not typically
a real problem.
The one caveat I would consider is if we wanted to let a multi-pack-index
cover thin packs, but that would be a substantial change to the object
database that I do not plan to tackle any time soon.
>> I'm also a little curious how bad it is to have a midx whose pack has
>> gone away. I guess we'd answer queries for "yes, we have this object"
>> even if we don't, which is bad. Though in practice we'd only delete
>> those packs if we have their objects elsewhere.
>
> Hmph, object O used to be in pack A and pack B, midx points at the
> copy in pack B but we deleted it because the pack is redundant.
> Now, midx says O exists, but midx cannot be used to retrieve O. We
> need to ask A.idx about O to locate it.
>
> That sounds brittle. I am not sure our error fallback is all that
> patient.
The best I can see is that prepare_midx_pack() will return 1 if the
pack no longer exists, and this would cause a die("error preparing
packfile from multi-pack-index") in nth_midxed_pack_entry(), killing
the following stack trace:
+ find_pack_entry():packfile.c
+ fill_midx_entry():midx.c
+ nth_midxed_pack_entry():midx.c
Perhaps that die() is a bit over-zealous since we could return 1
through this stack and find_pack_entry() could continue searching
for the object in the packed_git list. However, it could start
returning false _negatives_ if there were duplicates of the object
in the multi-pack-index but only the latest copy was deleted (and
the object does not appear in a pack-file outside of the multi-
pack-index).
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Jeff King @ 2020-08-25 16:47 UTC (permalink / raw)
To: Taylor Blau; +Cc: Son Luong Ngoc, git, dstolee
In-Reply-To: <20200825144515.GB7671@syl.lan>
On Tue, Aug 25, 2020 at 10:45:15AM -0400, Taylor Blau wrote:
> > I wonder if its worth to trigger write_midx_file() to update the midx instead of
> > just removing MIDX?
>
> There's no reason that we couldn't do this, but I don't think that it's
> a very good idea, especially if the new 'git maintenance' command will
> be able to do something like this by itself.
>
> I'm hesitant to add yet another option to 'git repack', which I have
> always thought as a plumbing tool. That's important because callers
> (like 'git maintenance' or user scripts) can 'git multi-pack-index write
> ...' after their 'git repack' to generate a new MIDX if they want one.
It may be worth thinking a bit about atomicity here, though. Rather than
separate delete and write steps, would somebody want a sequence like:
- we have a midx with packs A, B, C
- we find out that pack C is redundant and want to drop it
- we create a new midx with A and B in a tempfile
- we atomically rename the new midx over the old
- we delete pack C
A simultaneous reader always sees a consistent midx. Whereas in a
delete-then-rewrite scenario there is a moment where there is no midx,
and they'd fall back to reading the individual idx files.
It may not matter all that much, though, for two reasons:
- reading individual idx files should still give a correct answer.
It just may be a bit slower.
- even with an atomic replacement, I think caching on the reader side
may cause interesting effects. For instance, if a reader process
opens the old midx, it will generally cache that knowledge in memory
(including mmap the content). So even after the replacement and
deletion of C, it may still try to use the old midx that references
C.
If we do care, though, that implies some cooperation between the
deletion process and the midx code. Perhaps it even argues that repack
should refuse to delete such a single pack at all, since it _isn't_
redundant. It's part of a midx, and the caller should rewrite the midx
first itself, and _then_ look for redundant packs.
-Peff
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary
From: Jeff King @ 2020-08-25 16:56 UTC (permalink / raw)
To: Taylor Blau; +Cc: Derrick Stolee, git, dstolee
In-Reply-To: <20200825154224.GA9116@syl.lan>
On Tue, Aug 25, 2020 at 11:42:24AM -0400, Taylor Blau wrote:
> > test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '
> > git init preserve &&
> > test_when_finished "rm -fr preserve" &&
> > (
> > cd preserve &&
> > midx=.git/objects/pack/multi-pack-index &&
> >
> > test_commit 1 &&
> > HASH1=$(git pack-objects --all .git/objects/pack/pack) &&
> > touch .git/objects/pack/pack-$HASH1.keep &&
> >
> > cat >pack-input <<-\EOF &&
>
> Escaping the heredoc shouldn't be necessary, so this can be written
> instead as '<<-EOF'.
We usually go the opposite way: if a here-doc doesn't need
interpolation, then we prefer to mark it as such to avoid surprising
somebody who adds lines later (and might need quoting).
Certainly you can argue that least-surprise would be in the other
direction (i.e., people expect to interpolate by default, and you
surprise anybody adding a variable reference), but for Git's test suite,
the convention usually runs the other way.
-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