* Re: [PATCH v2] sequencer: remove unreachable exit condition in pick_commits()
From: Junio C Hamano @ 2023-09-13 0:32 UTC (permalink / raw)
To: Phillip Wood; +Cc: Oswald Buddenhagen, git, Johannes Schindelin
In-Reply-To: <7ede7c26-9029-4e4b-81a3-f992eff74124@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 12/09/2023 11:55, Oswald Buddenhagen wrote:
>> This was introduced by 56dc3ab04 ("sequencer (rebase -i): implement the
>> 'edit' command", 2017-01-02), and was pointless from the get-go: all
>> early exits from the loop above are returns, so todo_list->current ==
>> todo_list->nr is an invariant after the loop.
>> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> Thanks for updating the commit message, I think it is clearer now
Thanks, both. Queued.
^ permalink raw reply
* Re: [PATCH v2 2/2] completion: commit: complete trailers tokens more robustly
From: Junio C Hamano @ 2023-09-13 0:34 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget
Cc: git, ZheNing Hu, Martin Ågren, Philippe Blain
In-Reply-To: <9cb33c20294e096f5345fb1ea9d80a23e899ae6f.1694539827.git.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> In the previous commit, we added support for completing configured
> trailer tokens in 'git commit --trailer'.
>
> Make the implementation more robust by:
>
> - using '__git' instead of plain 'git', as the rest of the completion
> script does
> - using a stricter pattern for --get-regexp to avoid false hits
> - using 'cut' and 'rev' instead of 'awk' to account for tokens including
> dots.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
Thanks, both, for a quick fix.
Queued.
^ permalink raw reply
* Re: [PATCH v2] completion: improve doc for complex aliases
From: Junio C Hamano @ 2023-09-13 0:58 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget
Cc: git, Steffen Prohaska, Eric Sunshine, Linus Arver, Philippe Blain
In-Reply-To: <pull.1585.v2.git.1694538135853.gitgitgadget@gmail.com>
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> The completion code can be told to use a particular completion for
> aliases that shell out by using ': git <cmd> ;' as the first command of
> the alias. This only works if <cmd> and the semicolon are separated by a
> space, since if the space is missing __git_aliased_command returns (for
> example) 'checkout;' instead of just 'checkout', and then
> __git_complete_command fails to find a completion for 'checkout;'.
>
> The examples have that space but it's not clear if it's just for
> style or if it's mandatory. Explicitly mention it.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
Thanks. I scanned the case statement in the loop in the function
and thought "hmph, everybody says ': git <cmd> ;' but is 'git'
really needed?"
I had "git l3" alias that invokes "$HOM#/bin/git-l" command, like so:
[alias]
l3 = "!sh -c ': git log ; git l \"$@\"' -"
but if I did 's/: git log/: log/' it still completes just fine.
I wonder if this hack is worth adding, instead of (or in addition
to) requiring the user to insert $IFS to please the "parser", we can
honor the rather obvious wish of the user in a more direct way.
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git c/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
index 19139ac121..e31d71955f 100644
--- c/contrib/completion/git-completion.bash
+++ w/contrib/completion/git-completion.bash
@@ -1183,7 +1183,7 @@ __git_aliased_command ()
:) : skip null command ;;
\'*) : skip opening quote after sh -c ;;
*)
- cur="$word"
+ cur="${word%;}"
break
esac
done
^ permalink raw reply related
* Re: [PATCH v3 02/32] doc hash-function-transition: Augment compatObjectFormat with readCompatMap
From: Oswald Buddenhagen @ 2023-09-13 8:10 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Junio C Hamano, brian m. carlson, git
In-Reply-To: <87msxr8uc1.fsf@email.froward.int.ebiederm.org>
On Tue, Sep 12, 2023 at 07:11:26AM -0500, Eric W. Biederman wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> On Mon, Sep 11, 2023 at 06:46:19PM -0500, Eric W. Biederman wrote:
>>>+The difference between compatObjectFormat and readCompatMap would be that
>>>+compatObjectFormat would ask git to read existing maps, but would not ask
>>>+git to write or create them.
>>>
>> the argument makes sense, but the asymmetry in the naming bugs me. in particular
>> "[read]compatMap" seems too non-descript.
>
>I am open to suggestions for better names.
>
isn't readCompatObjectFormat an obvious choice?
(and for symmetry, the other then would be writeCompatObjectFormat, i
guess.)
regards
^ permalink raw reply
* Re: [PATCH v2] revision: add `--ignore-missing-links` user option
From: Karthik Nayak @ 2023-09-13 9:32 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, gitster
In-Reply-To: <ZQCa3GHT3D7aibJ1@nand.local>
On Tue, Sep 12, 2023 at 7:07 PM Taylor Blau <me@ttaylorr.com> wrote:
> > +# We create 5 commits and move them to the alt directory and
> > +# create 5 more commits which will stay in the main odb.
> > +test_expect_success 'create repository and alternate directory' '
> > + git init main &&
>
> We don't necessarily have to initialize a repository, as the test suite
> already does so for us. So we may want to write this instead as:
>
> test_commit_bulk 5 &&
> git clone --reference=. --shared . alt &&
> test_commit_bulk -C alt --start=6 5
>
I was trying to use the env variable `GIT_ALTERNATE_OBJECT_DIRECTORIES` and
get hence ended up creating a new repository. But I really like the
convenience functions
that you've suggested below. With that, this seems like the way to go.
> > +# when the alternate odb is provided, all commits are listed along with the boundary
> > +# commit.
> > +test_expect_success 'rev-list passes with alternate object directory' '
> > + GIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt git -C main rev-list HEAD >actual &&
> > + test_stdout_line_count = 10 cat actual &&
> > + grep $BOUNDARY_COMMIT actual
> > +'
>
> Here, I think we'd want to make sure that we have not just 10 lines of
> output, but that they are the 10 that we expect, like so:
>
> git -C alt rev-list --all --objects --no-object-names >actual.raw &&
> {
> git rev-list --all --objects --no-object-names &&
> git -C alt rev-list --all --objects --no-object-names --not \
> --alternate-refs
> } >expect.raw &&
> sort actual.raw >actual &&
> sort expect.raw >expect &&
> test_cmp expect actual
>
> When reviewing this and tweaking some of the tests locally, I found it
> useful to have some convenience functions like "hide_alternates" and
> "show_alternates" to control whether or not "alt" could see its
> alternate or not.
>
> From my review locally, the resulting changes (which can be applied
> directly on top of your patch here look like):
>
This is much better. I didn't know about `test_oid_to_path` and
`test_when_finished`, and overall
your patch looks much nicer and is more thorough in the testing. I'll
add it to the next version.
Will wait a day or two for more feedback before I submit v3.
Thanks again for your review and the patch :)
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-13 9:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git, Alexey Gladkov
In-Reply-To: <xmqq1qf2lxrm.fsf@gitster.g>
I think I should trim CC to not spam the people who are not
interested in this discussion...
On 09/12, Junio C Hamano wrote:
>
> Documentation may not match the behaviour, but do we know what the
> behaviour we want is? To me, the last example that shows the same
> line twice (one as a real hit, the other because of "-p") looks a
> bit counter-intuitive for the purpose of "help me locate where the
> grep hits are". I dunno.
I have another opinion. To me the 2nd "=..." marker does help to
understand the hit location. But this doesn't matter.
Let me repeat: scripts.
I tried to explain this in 0/1 and in my other replies, but lets
start from the very beginning once again.
I've never used git-grep with -p/-n and most probably never will.
But 3 days ago my text editor (vi clone) started to use "grep -pn".
$ cat -n TEST.c
1 void func1(struct pid *);
2
3 void func2(struct pid *pid)
4 {
5 use1(pid);
6 }
7
8 void func3(struct pid *pid)
9 {
10 use2(pid);
11 }
when I do
:git-grep --untracked -pn pid TEST.c
in my editor it calls the script which parses the output from git-grep
and puts this
<pre>
<a href="TEST.c?1">TEST.c </a> 1 <b> </b> void func1(struct <i>pid</i> *);
<a href="TEST.c?3">TEST.c </a> 3 <b> </b> void func2(struct <i>pid</i> *<i>pid</i>)
<a href="TEST.c?5">TEST.c </a> 5 <b>func2 </b> use1(<i>pid</i>);
<a href="TEST.c?8">TEST.c </a> 8 <b> </b> void func3(struct <i>pid</i> *<i>pid</i>)
<a href="TEST.c?10">TEST.c </a> 10 <b>func3 </b> use2(<i>pid</i>);
</pre>
html to the text buffer which is nicely displayed as
TEST.c 1 void func1(struct pid *);
TEST.c 3 void func2(struct pid *pid)
TEST.c 5 func2 use1(pid);
TEST.c 8 void func3(struct pid *pid)
TEST.c 10 func3 use2(pid);
and I can use Ctrl-] to jump to the file/function/location.
And this script is very simple, it parses the output line-by-line. When
it sees the "=" marker it does some minimal post-processing, records the
function name to display it in the 3rd column later, and goes to the next
line.
But without my patch, in this case I get
TEST.c 1 void func1(struct pid *);
TEST.c 3 void func2(struct pid *pid)
TEST.c 5 use1(pid);
TEST.c 8 void func3(struct pid *pid)
TEST.c 10 use2(pid);
because the output from git-grep
$ git grep --untracked -pn pid TEST.c
TEST.c:1:void func1(struct pid *);
TEST.c:3:void func2(struct pid *pid)
TEST.c:5: use1(pid);
TEST.c:8:void func3(struct pid *pid)
TEST.c:10: use2(pid);
doesn't have the "=..." markers at all.
But TEST.c is just the trivial/artificial example. From 0/1,
When I do
:git-grep -pw pid kernel/sys.c
in my editor without this patch, I get
kernel/sys.c 224 sys_setpriority struct pid *pgrp;
kernel/sys.c 294 sys_getpriority struct pid *pgrp;
kernel/sys.c 952 * Note, despite the name, this returns the tgid not the pid. The tgid and
kernel/sys.c 953 * the pid are identical unless CLONE_THREAD was specified on clone() in
kernel/sys.c 963 /* Thread ID - the internal kernel "pid" */
kernel/sys.c 977 sys_getppid int pid;
kernel/sys.c 980 sys_getppid pid = task_tgid_vnr(rcu_dereference(current->real_parent));
kernel/sys.c 983 sys_getppid return pid;
kernel/sys.c 1073 SYSCALL_DEFINE2(setpgid, pid_t, pid, pid_t, pgid)
kernel/sys.c 1077 sys_times struct pid *pgrp;
kernel/sys.c 1080 sys_times if (!pid)
kernel/sys.c 1081 sys_times pid = task_pid_vnr(group_leader);
kernel/sys.c 1083 sys_times pgid = pid;
kernel/sys.c 1094 sys_times p = find_task_by_vpid(pid);
kernel/sys.c 1120 sys_times if (pgid != pid) {
kernel/sys.c 1144 static int do_getpgid(pid_t pid)
kernel/sys.c 1147 sys_times struct pid *grp;
kernel/sys.c 1151 sys_times if (!pid)
kernel/sys.c 1155 sys_times p = find_task_by_vpid(pid);
kernel/sys.c 1172 SYSCALL_DEFINE1(getpgid, pid_t, pid)
kernel/sys.c 1174 sys_times return do_getpgid(pid);
kernel/sys.c 1186 SYSCALL_DEFINE1(getsid, pid_t, pid)
kernel/sys.c 1189 sys_getpgrp struct pid *sid;
kernel/sys.c 1193 sys_getpgrp if (!pid)
kernel/sys.c 1197 sys_getpgrp p = find_task_by_vpid(pid);
kernel/sys.c 1214 static void set_special_pids(struct pid *pid)
kernel/sys.c 1218 sys_getpgrp if (task_session(curr) != pid)
kernel/sys.c 1219 sys_getpgrp change_pid(curr, PIDTYPE_SID, pid);
kernel/sys.c 1221 sys_getpgrp if (task_pgrp(curr) != pid)
kernel/sys.c 1222 sys_getpgrp change_pid(curr, PIDTYPE_PGID, pid);
kernel/sys.c 1228 ksys_setsid struct pid *sid = task_pid(group_leader);
kernel/sys.c 1684 SYSCALL_DEFINE4(prlimit64, pid_t, pid, unsigned int, resource,
kernel/sys.c 1705 check_prlimit_permission tsk = pid ? find_task_by_vpid(pid) : current;
And only the first 5 funcnames are correct.
And note that this case is very simple too (I mostly use :git-grep to scan
the whole linux kernel tree), but even in this simple case I don't think it
makes sense to use "git-grep -pn" directly, the output is hardly readable
(at least to me) with or without my patch.
Oleg.
^ permalink raw reply
* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-13 10:15 UTC (permalink / raw)
To: René Scharfe; +Cc: Junio C Hamano, git, Alexey Gladkov
In-Reply-To: <df05f761-c498-6930-bfd8-265f7e23d8ee@web.de>
On 09/12, René Scharfe wrote:
>
> >> So perhaps git-grep needs another change, something like
> >>
> >> if (match_funcname(opt, gs, bol, end_of_line(...)))
> >> return;
> >>
> >> at the start of show_funcname_line(), but my patch does not change this
> >> behaviour.
>
> Yes, to make it match the documentation it would need something like
> that. (Though I'd add a match_funcname() call before the
> show_funcname_line() call in grep_source_1() instead, as it already has
> the eol value.)
Yes, I too thought about this. Except I thought that it makes sense to
pass the additional "unsigned eol" argument to show_funcname_line().
But in any case show_pre_context() will need to calculate eol.
However this is just a minor detail, I am fine either way.
> > So, just in case, please see V2 below. In my opinion it _fixes_ the
> > current behaviour. With this patch
> >
> > $ ./git grep --untracked -pn func2 TEST1.c
> > TEST1.c:4:void func2()
>
> Indeed that matches the letter of the documentation.
>
> > $ ./git grep --untracked -pn xxx TEST2.c
> > TEST2.c:1:void func(xxx)
> > TEST2.c=1=void func(xxx)
> > TEST2.c:3: use(xxx);
>
> That one as well.
So. Can I assume you agree with my patch ? ;)
> No, I think the documentation is wrong.
Well, to me it looks good, but only after this patch.
Oleg.
^ permalink raw reply
* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Phillip Wood @ 2023-09-13 15:16 UTC (permalink / raw)
To: Jeff King, phillip.wood; +Cc: git, Johannes Schindelin
In-Reply-To: <20230911093616.GA1605460@coredump.intra.peff.net>
Hi Peff
On 11/09/2023 10:36, Jeff King wrote:
> On Thu, Sep 07, 2023 at 11:04:33AM +0100, Phillip Wood wrote:
> Looking at the ci-config branch of phillipwood/git.git, I see this in
> your allow-refs:
>
> refs/heads/main|refs/heads/master|refs/heads/maint|refs/heads/next|refs/heads/seen|refs/tags/gitgui-*|refs/tags/pr-[0-9]*|refs/tags/v[1-9].*)
>
> So you do use multiple prefixes, though all in refs/tags/. Do you
> actually push tags for which you do want to run CI,
Yes, but not very often - I could probably just reject all tags and
start the CI manually when I want it (assuming that's an option). Thanks
for digging into the various options, it sounds like it is possible so
long as we don't want multiple prefixes.
Aside: what I'd really like is to be able to set an environment variable
when I push to skip or force the CI
GITHUB_SKIP_CI=1 git push github ...
but that would require support from the git client, the protocol and the
server.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Phillip Wood @ 2023-09-13 15:33 UTC (permalink / raw)
To: Oswald Buddenhagen, phillip.wood
Cc: Jeff King, Phillip Wood via GitGitGadget, git,
Johannes Schindelin, Junio C Hamano
In-Reply-To: <ZP7st70/G3kLujHG@ugly>
On 11/09/2023 11:32, Oswald Buddenhagen wrote:
> On Mon, Sep 11, 2023 at 11:14:31AM +0100, Phillip Wood wrote:
>> On 11/09/2023 11:00, Phillip Wood wrote:
>>> There is an inevitable race between wait() returning and calling
>>> signal() to restore the handlers for SIGINT and SIGQUIT,
>>
>> In principle if we installed a signal handler to set a flag if a
>> signal is received while calling wait() and then once wait() returns
>> successfully see if the child was killed we can tell if the signal was
>> received while the child was alive.
>>
> yes, this is what i was already writing:
I'm afraid that was not clear to me from your message.
> my point is that you shouldn't be doing that in the first place.
> install the handlers when the sequencer is entered and leave them there.
> the handlers need to set (volatile) flag variables, which are checked by
> the sequencer on a regular basis.
I did consider doing that before I submitted this patch but it is a much
more invasive and substantial change. The patch here makes it safe for
the user to interrupt a subprocess started by the sequencer. If I
understand correctly your suggestion implies that the user could
interrupt the sequencer at any point and we'd need to exit and ensure
that they could safely continue the rebase afterwards. That is not the
case at the moment and I'm concerned making that promise could turn into
a maintenance burden in the future.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Oswald Buddenhagen @ 2023-09-13 16:40 UTC (permalink / raw)
To: phillip.wood
Cc: Jeff King, Phillip Wood via GitGitGadget, git,
Johannes Schindelin, Junio C Hamano
In-Reply-To: <51ad269b-d8df-4424-8127-293b2971d65d@gmail.com>
On Wed, Sep 13, 2023 at 04:33:06PM +0100, Phillip Wood wrote:
>On 11/09/2023 11:32, Oswald Buddenhagen wrote:
>> On Mon, Sep 11, 2023 at 11:14:31AM +0100, Phillip Wood wrote:
>>> On 11/09/2023 11:00, Phillip Wood wrote:
>>>> There is an inevitable race between wait() returning and calling
>>>> signal() to restore the handlers for SIGINT and SIGQUIT,
>>>
>>> In principle if we installed a signal handler to set a flag if a
>>> signal is received while calling wait() and then once wait() returns
>>> successfully see if the child was killed we can tell if the signal was
>>> received while the child was alive.
>>>
>> yes, this is what i was already writing:
>
>I'm afraid that was not clear to me from your message.
>
i meant, this is what i already wrote before i read your reply-to-self.
i just pasted it into the new reply i sent instead without adjusting for
the new context. the sentence was meant to explain the slight "impedance
mismatch".
>> install the handlers when the sequencer is entered and leave them
>> there. the handlers need to set (volatile) flag variables, which are
>> checked by the sequencer on a regular basis.
>
>I did consider doing that before I submitted this patch
>but it is a much more invasive and substantial change.
>
yes
>The patch here makes it safe for the user to interrupt a subprocess
>started by the sequencer.
>
for the exec case, i don't see how this actually improves anything.
whether git gets killed along with the child, or catches the child's
abnormal exit and immediately exits, makes no difference. arguably, it's
even counter-productive, because from the outside it's random whether
git will just exit on sigint or report that its child exited on sigint
and exit with some other status.
actual value would come from doing something before exiting, but the
commit message is pretty much saying that this is not the case.
the commit/edit case is more complicated, but arguably the problem is
the (hypothetical?) editor that just ignores sigint rather than
reprogramming the terminal appropriately for full-screen use.
git-commit ignoring sigint seems like a somewhat misguided workaround,
and piling on top of that won't really improve things.
>If I understand correctly your suggestion implies that the user could
>interrupt the sequencer at any point and we'd need to exit and ensure
>that they could safely continue the rebase afterwards.
>
yes
>That is not the case at the moment
>and I'm concerned making that promise could turn into a maintenance
>burden in the future.
>
of course it would. the question is whether it would be worth it. with
delayed state commits, some extra trasactionality might well be
required.
regards
^ permalink raw reply
* Re: [PATCH v2] revision: add `--ignore-missing-links` user option
From: Taylor Blau @ 2023-09-13 17:17 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
In-Reply-To: <CAOLa=ZSVobP9Be9W3f1BWoZjLUYwOqOpCQMCXVH34=iGKfXnmw@mail.gmail.com>
On Wed, Sep 13, 2023 at 11:32:13AM +0200, Karthik Nayak wrote:
> > From my review locally, the resulting changes (which can be applied
> > directly on top of your patch here look like):
>
> This is much better. I didn't know about `test_oid_to_path` and
> `test_when_finished`, and overall your patch looks much nicer and is
> more thorough in the testing. I'll add it to the next version.
Thanks for folding it in! I felt bad that I might have stepped on your
toes by saying "here's how I would do it", but by the time I had applied
your patch locally to review it, I had already generated the
aforementioned diff.
> Will wait a day or two for more feedback before I submit v3.
Sounds like a plan :-).
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v7 0/3] Add unit test framework and project plan
From: Junio C Hamano @ 2023-09-13 18:14 UTC (permalink / raw)
To: Josh Steadmon; +Cc: git, linusa, calvinwan, phillip.wood123, rsbecker
In-Reply-To: <ZOe5gtYeQaKqOJVm@google.com>
Josh Steadmon <steadmon@google.com> writes:
> BTW, I'm going to be AFK for a couple weeks, so it will be a while
> before I'm able to address feedback on this series. Thanks in advance.
OK. Enjoy your time off.
^ permalink raw reply
* [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Taylor Blau @ 2023-09-13 19:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1693946195.git.me@ttaylorr.com>
Here is a small reroll of my series to clean up some of the internals of
'git repack' used to track the set of existing packs.
Much is unchanged from the last round, save for some additional clean-up
on how we handle the '->util' field for each pack's string_list_item in
response to very helpful review from those CC'd.
As usual, a range-diff is available below for convenience. Thanks in
advance for your review!
Taylor Blau (8):
builtin/repack.c: extract structure to store existing packs
builtin/repack.c: extract marking packs for deletion
builtin/repack.c: extract redundant pack cleanup for --geometric
builtin/repack.c: extract redundant pack cleanup for existing packs
builtin/repack.c: extract `has_existing_non_kept_packs()`
builtin/repack.c: store existing cruft packs separately
builtin/repack.c: avoid directly inspecting "util"
builtin/repack.c: extract common cruft pack loop
builtin/repack.c | 293 +++++++++++++++++++++++++++++------------------
1 file changed, 180 insertions(+), 113 deletions(-)
Range-diff against v1:
1: 5b48b7e3cc = 1: 2e26beff22 builtin/repack.c: extract structure to store existing packs
2: 313537ef68 = 2: 62d916169d builtin/repack.c: extract marking packs for deletion
3: 5c25ef87c1 = 3: 7ed45804ea builtin/repack.c: extract redundant pack cleanup for --geometric
4: 7bb543fef8 = 4: 82057de4cf builtin/repack.c: extract redundant pack cleanup for existing packs
5: e2cf87bb94 = 5: f4f7b4c08f builtin/repack.c: extract `has_existing_non_kept_packs()`
6: 414a558883 = 6: d68a88dbd5 builtin/repack.c: store existing cruft packs separately
7: 559b487e2a ! 7: 481a29599b builtin/repack.c: drop `DELETE_PACK` macro
@@ Metadata
Author: Taylor Blau <me@ttaylorr.com>
## Commit message ##
- builtin/repack.c: drop `DELETE_PACK` macro
+ builtin/repack.c: avoid directly inspecting "util"
+ The `->util` field corresponding to each string_list_item is used to
+ track the existence of some pack at the beginning of a repack operation
+ was originally intended to be used as a bitfield.
+
+ This bitfield tracked:
+
+ - (1 << 0): whether or not the pack should be deleted
+ - (1 << 1): whether or not the pack is cruft
+
+ The previous commit removed the use of the second bit, but a future
+ patch (from a different series than this one) will introduce a new use
+ of it.
+
+ So we could stop treating the util pointer as a bitfield and instead
+ start treating it as if it were a boolean. But this would require some
+ backtracking when that later patch is applied.
+
+ Instead, let's avoid touching the ->util field directly, and instead
+ introduce convenience functions like:
+
+ - pack_mark_for_deletion()
+ - pack_is_marked_for_deletion()
+
+ Helped-by: Junio C Hamano <gitster@pobox.com>
+ Helped-by: Jeff King <peff@peff.net>
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## builtin/repack.c ##
-@@
- #define LOOSEN_UNREACHABLE 2
- #define PACK_CRUFT 4
+@@ builtin/repack.c: static int has_existing_non_kept_packs(const struct existing_packs *existing)
+ return existing->non_kept_packs.nr || existing->cruft_packs.nr;
+ }
--#define DELETE_PACK 1
--
- static int pack_everything;
- static int delta_base_offset = 1;
- static int pack_kept_objects = -1;
-@@ builtin/repack.c: static int repack_config(const char *var, const char *value,
-
- struct existing_packs {
- struct string_list kept_packs;
-+ /*
-+ * for both non_kept_packs, and cruft_packs, a non-NULL
-+ * 'util' field indicates the pack should be deleted.
-+ */
- struct string_list non_kept_packs;
- struct string_list cruft_packs;
- };
++static void pack_mark_for_deletion(struct string_list_item *item)
++{
++ item->util = (void*)((uintptr_t)item->util | DELETE_PACK);
++}
++
++static int pack_is_marked_for_deletion(struct string_list_item *item)
++{
++ return (uintptr_t)item->util & DELETE_PACK;
++}
++
+ static void mark_packs_for_deletion_1(struct string_list *names,
+ struct string_list *list)
+ {
@@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *names,
* (if `-d` was given).
*/
if (!string_list_has_string(names, sha1))
- item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
-+ item->util = (void*)1;
++ pack_mark_for_deletion(item);
}
}
@@ builtin/repack.c: static void remove_redundant_packs_1(struct string_list *packs
struct string_list_item *item;
for_each_string_list_item(item, packs) {
- if (!((uintptr_t)item->util & DELETE_PACK))
-+ if (!item->util)
++ if (!pack_is_marked_for_deletion(item))
continue;
remove_redundant_pack(packdir, item->string);
}
@@ builtin/repack.c: static void midx_included_packs(struct string_list *include,
-
- for_each_string_list_item(item, &existing->cruft_packs) {
- /*
-- * no need to check DELETE_PACK, since we're not
-- * doing an ALL_INTO_ONE repack
-+ * no need to check for deleted packs, since we're
-+ * not doing an ALL_INTO_ONE repack
- */
- string_list_insert(include, xstrfmt("%s.idx", item->string));
}
} else {
for_each_string_list_item(item, &existing->non_kept_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
-+ if (item->util)
++ if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
for_each_string_list_item(item, &existing->cruft_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
-+ if (item->util)
++ if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
8: ca7d13e7bf ! 8: 68748eb9c8 builtin/repack.c: extract common cruft pack loop
@@ Commit message
## builtin/repack.c ##
@@ builtin/repack.c: static void midx_included_packs(struct string_list *include,
+
string_list_insert(include, strbuf_detach(&buf, NULL));
}
-
+-
- for_each_string_list_item(item, &existing->cruft_packs) {
- /*
-- * no need to check for deleted packs, since we're
-- * not doing an ALL_INTO_ONE repack
+- * no need to check DELETE_PACK, since we're not
+- * doing an ALL_INTO_ONE repack
- */
- string_list_insert(include, xstrfmt("%s.idx", item->string));
- }
} else {
for_each_string_list_item(item, &existing->non_kept_packs) {
- if (item->util)
+ if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
+ }
- for_each_string_list_item(item, &existing->cruft_packs) {
-- if (item->util)
+- if (pack_is_marked_for_deletion(item))
- continue;
- string_list_insert(include, xstrfmt("%s.idx", item->string));
- }
@@ builtin/repack.c: static void midx_included_packs(struct string_list *include,
+ * `mark_packs_for_deletion()` when doing an all-into-one
+ * repack).
+ */
-+ if (item->util)
++ if (pack_is_marked_for_deletion(item))
+ continue;
+ string_list_insert(include, xstrfmt("%s.idx", item->string));
}
--
2.42.0.166.g68748eb9c8
^ permalink raw reply
* [PATCH v2 1/8] builtin/repack.c: extract structure to store existing packs
From: Taylor Blau @ 2023-09-13 19:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
The repack machinery needs to keep track of which packfiles were present
in the repository at the beginning of a repack, segmented by whether or
not each pack is marked as kept.
The names of these packs are stored in two `string_list`s, corresponding
to kept- and non-kept packs, respectively. As a consequence, many
functions within the repack code need to take both `string_list`s as
arguments, leading to code like this:
ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,
cruft_expiration, &names,
&existing_nonkept_packs, /* <- */
&existing_kept_packs); /* <- */
Wrap up this pair of `string_list`s into a single structure that stores
both. This saves us from having to pass both string lists separately,
and prepares for adding additional fields to this structure.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 90 ++++++++++++++++++++++++++----------------------
1 file changed, 49 insertions(+), 41 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 6943c5ba11..67bf86567f 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -95,14 +95,29 @@ static int repack_config(const char *var, const char *value,
return git_default_config(var, value, ctx, cb);
}
+struct existing_packs {
+ struct string_list kept_packs;
+ struct string_list non_kept_packs;
+};
+
+#define EXISTING_PACKS_INIT { \
+ .kept_packs = STRING_LIST_INIT_DUP, \
+ .non_kept_packs = STRING_LIST_INIT_DUP, \
+}
+
+static void existing_packs_release(struct existing_packs *existing)
+{
+ string_list_clear(&existing->kept_packs, 0);
+ string_list_clear(&existing->non_kept_packs, 0);
+}
+
/*
- * Adds all packs hex strings (pack-$HASH) to either fname_nonkept_list
- * or fname_kept_list based on whether each pack has a corresponding
+ * Adds all packs hex strings (pack-$HASH) to either packs->non_kept
+ * or packs->kept based on whether each pack has a corresponding
* .keep file or not. Packs without a .keep file are not to be kept
* if we are going to pack everything into one file.
*/
-static void collect_pack_filenames(struct string_list *fname_nonkept_list,
- struct string_list *fname_kept_list,
+static void collect_pack_filenames(struct existing_packs *existing,
const struct string_list *extra_keep)
{
struct packed_git *p;
@@ -126,16 +141,16 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,
strbuf_strip_suffix(&buf, ".pack");
if ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)
- string_list_append(fname_kept_list, buf.buf);
+ string_list_append(&existing->kept_packs, buf.buf);
else {
struct string_list_item *item;
- item = string_list_append(fname_nonkept_list, buf.buf);
+ item = string_list_append(&existing->non_kept_packs, buf.buf);
if (p->is_cruft)
item->util = (void*)(uintptr_t)CRUFT_PACK;
}
}
- string_list_sort(fname_kept_list);
+ string_list_sort(&existing->kept_packs);
strbuf_release(&buf);
}
@@ -327,7 +342,7 @@ static int geometry_cmp(const void *va, const void *vb)
}
static void init_pack_geometry(struct pack_geometry *geometry,
- struct string_list *existing_kept_packs,
+ struct existing_packs *existing,
const struct pack_objects_args *args)
{
struct packed_git *p;
@@ -344,23 +359,24 @@ static void init_pack_geometry(struct pack_geometry *geometry,
if (!pack_kept_objects) {
/*
- * Any pack that has its pack_keep bit set will appear
- * in existing_kept_packs below, but this saves us from
- * doing a more expensive check.
+ * Any pack that has its pack_keep bit set will
+ * appear in existing->kept_packs below, but
+ * this saves us from doing a more expensive
+ * check.
*/
if (p->pack_keep)
continue;
/*
- * The pack may be kept via the --keep-pack option;
- * check 'existing_kept_packs' to determine whether to
- * ignore it.
+ * The pack may be kept via the --keep-pack
+ * option; check 'existing->kept_packs' to
+ * determine whether to ignore it.
*/
strbuf_reset(&buf);
strbuf_addstr(&buf, pack_basename(p));
strbuf_strip_suffix(&buf, ".pack");
- if (string_list_has_string(existing_kept_packs, buf.buf))
+ if (string_list_has_string(&existing->kept_packs, buf.buf))
continue;
}
if (p->is_cruft)
@@ -565,14 +581,13 @@ static void midx_snapshot_refs(struct tempfile *f)
}
static void midx_included_packs(struct string_list *include,
- struct string_list *existing_nonkept_packs,
- struct string_list *existing_kept_packs,
+ struct existing_packs *existing,
struct string_list *names,
struct pack_geometry *geometry)
{
struct string_list_item *item;
- for_each_string_list_item(item, existing_kept_packs)
+ for_each_string_list_item(item, &existing->kept_packs)
string_list_insert(include, xstrfmt("%s.idx", item->string));
for_each_string_list_item(item, names)
string_list_insert(include, xstrfmt("pack-%s.idx", item->string));
@@ -600,7 +615,7 @@ static void midx_included_packs(struct string_list *include,
string_list_insert(include, strbuf_detach(&buf, NULL));
}
- for_each_string_list_item(item, existing_nonkept_packs) {
+ for_each_string_list_item(item, &existing->non_kept_packs) {
if (!((uintptr_t)item->util & CRUFT_PACK)) {
/*
* no need to check DELETE_PACK, since we're not
@@ -611,7 +626,7 @@ static void midx_included_packs(struct string_list *include,
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
} else {
- for_each_string_list_item(item, existing_nonkept_packs) {
+ for_each_string_list_item(item, &existing->non_kept_packs) {
if ((uintptr_t)item->util & DELETE_PACK)
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
@@ -700,8 +715,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,
const char *pack_prefix,
const char *cruft_expiration,
struct string_list *names,
- struct string_list *existing_packs,
- struct string_list *existing_kept_packs)
+ struct existing_packs *existing)
{
struct child_process cmd = CHILD_PROCESS_INIT;
struct strbuf line = STRBUF_INIT;
@@ -743,9 +757,9 @@ static int write_cruft_pack(const struct pack_objects_args *args,
in = xfdopen(cmd.in, "w");
for_each_string_list_item(item, names)
fprintf(in, "%s-%s.pack\n", pack_prefix, item->string);
- for_each_string_list_item(item, existing_packs)
+ for_each_string_list_item(item, &existing->non_kept_packs)
fprintf(in, "-%s.pack\n", item->string);
- for_each_string_list_item(item, existing_kept_packs)
+ for_each_string_list_item(item, &existing->kept_packs)
fprintf(in, "%s.pack\n", item->string);
fclose(in);
@@ -777,8 +791,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list_item *item;
struct string_list names = STRING_LIST_INIT_DUP;
- struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
- struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
+ struct existing_packs existing = EXISTING_PACKS_INIT;
struct pack_geometry geometry = { 0 };
struct strbuf line = STRBUF_INIT;
struct tempfile *refs_snapshot = NULL;
@@ -914,13 +927,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
packtmp_name = xstrfmt(".tmp-%d-pack", (int)getpid());
packtmp = mkpathdup("%s/%s", packdir, packtmp_name);
- collect_pack_filenames(&existing_nonkept_packs, &existing_kept_packs,
- &keep_pack_list);
+ collect_pack_filenames(&existing, &keep_pack_list);
if (geometry.split_factor) {
if (pack_everything)
die(_("options '%s' and '%s' cannot be used together"), "--geometric", "-A/-a");
- init_pack_geometry(&geometry, &existing_kept_packs, &po_args);
+ init_pack_geometry(&geometry, &existing, &po_args);
split_pack_geometry(&geometry);
}
@@ -964,7 +976,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (pack_everything & ALL_INTO_ONE) {
repack_promisor_objects(&po_args, &names);
- if (existing_nonkept_packs.nr && delete_redundant &&
+ if (existing.non_kept_packs.nr && delete_redundant &&
!(pack_everything & PACK_CRUFT)) {
for_each_string_list_item(item, &names) {
strvec_pushf(&cmd.args, "--keep-pack=%s-%s.pack",
@@ -1055,8 +1067,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix,
cruft_expiration, &names,
- &existing_nonkept_packs,
- &existing_kept_packs);
+ &existing);
if (ret)
goto cleanup;
@@ -1087,8 +1098,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
pack_prefix,
NULL,
&names,
- &existing_nonkept_packs,
- &existing_kept_packs);
+ &existing);
if (ret)
goto cleanup;
}
@@ -1134,7 +1144,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (delete_redundant && pack_everything & ALL_INTO_ONE) {
const int hexsz = the_hash_algo->hexsz;
- for_each_string_list_item(item, &existing_nonkept_packs) {
+ for_each_string_list_item(item, &existing.non_kept_packs) {
char *sha1;
size_t len = strlen(item->string);
if (len < hexsz)
@@ -1153,8 +1163,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (write_midx) {
struct string_list include = STRING_LIST_INIT_NODUP;
- midx_included_packs(&include, &existing_nonkept_packs,
- &existing_kept_packs, &names, &geometry);
+ midx_included_packs(&include, &existing, &names, &geometry);
ret = write_midx_included_packs(&include, &geometry,
refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
@@ -1173,7 +1182,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (delete_redundant) {
int opts = 0;
- for_each_string_list_item(item, &existing_nonkept_packs) {
+ for_each_string_list_item(item, &existing.non_kept_packs) {
if (!((uintptr_t)item->util & DELETE_PACK))
continue;
remove_redundant_pack(packdir, item->string);
@@ -1194,7 +1203,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
strbuf_strip_suffix(&buf, ".pack");
if ((p->pack_keep) ||
- (string_list_has_string(&existing_kept_packs,
+ (string_list_has_string(&existing.kept_packs,
buf.buf)))
continue;
@@ -1225,8 +1234,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
cleanup:
string_list_clear(&names, 1);
- string_list_clear(&existing_nonkept_packs, 0);
- string_list_clear(&existing_kept_packs, 0);
+ existing_packs_release(&existing);
free_pack_geometry(&geometry);
return ret;
--
2.42.0.166.g68748eb9c8
^ permalink raw reply related
* [PATCH v2 2/8] builtin/repack.c: extract marking packs for deletion
From: Taylor Blau @ 2023-09-13 19:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
At the end of a repack (when given `-d`), Git attempts to remove any
packs which have been made "redundant" as a result of the repacking
operation. For example, an all-into-one (`-A` or `-a`) repack makes
every pre-existing pack which is not marked as kept redundant. Geometric
repacks (with `--geometric=<n>`) make any packs which were rolled up
redundant, and so on.
But before deleting the set of packs we think are redundant, we first
check to see whether or not we just wrote a pack which is identical to
any one of the packs we were going to delete. When this is the case, Git
must avoid deleting that pack, since it matches a pack we just wrote
(so deleting it may cause the repository to become corrupt).
Right now we only process the list of non-kept packs in a single pass.
But a future change will split the existing non-kept packs further into
two lists: one for cruft packs, and another for non-cruft packs.
Factor out this routine to prepare for calling it twice on two separate
lists in a future patch.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 50 +++++++++++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 18 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 67bf86567f..0eee430951 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -105,6 +105,36 @@ struct existing_packs {
.non_kept_packs = STRING_LIST_INIT_DUP, \
}
+static void mark_packs_for_deletion_1(struct string_list *names,
+ struct string_list *list)
+{
+ struct string_list_item *item;
+ const int hexsz = the_hash_algo->hexsz;
+
+ for_each_string_list_item(item, list) {
+ char *sha1;
+ size_t len = strlen(item->string);
+ if (len < hexsz)
+ continue;
+ sha1 = item->string + len - hexsz;
+ /*
+ * Mark this pack for deletion, which ensures that this
+ * pack won't be included in a MIDX (if `--write-midx`
+ * was given) and that we will actually delete this pack
+ * (if `-d` was given).
+ */
+ if (!string_list_has_string(names, sha1))
+ item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
+ }
+}
+
+static void mark_packs_for_deletion(struct existing_packs *existing,
+ struct string_list *names)
+
+{
+ mark_packs_for_deletion_1(names, &existing->non_kept_packs);
+}
+
static void existing_packs_release(struct existing_packs *existing)
{
string_list_clear(&existing->kept_packs, 0);
@@ -1142,24 +1172,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
}
/* End of pack replacement. */
- if (delete_redundant && pack_everything & ALL_INTO_ONE) {
- const int hexsz = the_hash_algo->hexsz;
- for_each_string_list_item(item, &existing.non_kept_packs) {
- char *sha1;
- size_t len = strlen(item->string);
- if (len < hexsz)
- continue;
- sha1 = item->string + len - hexsz;
- /*
- * Mark this pack for deletion, which ensures that this
- * pack won't be included in a MIDX (if `--write-midx`
- * was given) and that we will actually delete this pack
- * (if `-d` was given).
- */
- if (!string_list_has_string(&names, sha1))
- item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
- }
- }
+ if (delete_redundant && pack_everything & ALL_INTO_ONE)
+ mark_packs_for_deletion(&existing, &names);
if (write_midx) {
struct string_list include = STRING_LIST_INIT_NODUP;
--
2.42.0.166.g68748eb9c8
^ permalink raw reply related
* [PATCH v2 3/8] builtin/repack.c: extract redundant pack cleanup for --geometric
From: Taylor Blau @ 2023-09-13 19:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
To reduce the complexity of the already quite-long `cmd_repack()`
implementation, extract out the parts responsible for deleting redundant
packs from a geometric repack out into its own sub-routine.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 52 +++++++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 23 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 0eee430951..71366811e9 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -540,6 +540,32 @@ static struct packed_git *get_preferred_pack(struct pack_geometry *geometry)
return NULL;
}
+static void geometry_remove_redundant_packs(struct pack_geometry *geometry,
+ struct string_list *names,
+ struct existing_packs *existing)
+{
+ struct strbuf buf = STRBUF_INIT;
+ uint32_t i;
+
+ for (i = 0; i < geometry->split; i++) {
+ struct packed_git *p = geometry->pack[i];
+ if (string_list_has_string(names, hash_to_hex(p->hash)))
+ continue;
+
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, pack_basename(p));
+ strbuf_strip_suffix(&buf, ".pack");
+
+ if ((p->pack_keep) ||
+ (string_list_has_string(&existing->kept_packs, buf.buf)))
+ continue;
+
+ remove_redundant_pack(packdir, buf.buf);
+ }
+
+ strbuf_release(&buf);
+}
+
static void free_pack_geometry(struct pack_geometry *geometry)
{
if (!geometry)
@@ -1202,29 +1228,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
remove_redundant_pack(packdir, item->string);
}
- if (geometry.split_factor) {
- struct strbuf buf = STRBUF_INIT;
-
- uint32_t i;
- for (i = 0; i < geometry.split; i++) {
- struct packed_git *p = geometry.pack[i];
- if (string_list_has_string(&names,
- hash_to_hex(p->hash)))
- continue;
-
- strbuf_reset(&buf);
- strbuf_addstr(&buf, pack_basename(p));
- strbuf_strip_suffix(&buf, ".pack");
-
- if ((p->pack_keep) ||
- (string_list_has_string(&existing.kept_packs,
- buf.buf)))
- continue;
-
- remove_redundant_pack(packdir, buf.buf);
- }
- strbuf_release(&buf);
- }
+ if (geometry.split_factor)
+ geometry_remove_redundant_packs(&geometry, &names,
+ &existing);
if (show_progress)
opts |= PRUNE_PACKED_VERBOSE;
prune_packed_objects(opts);
--
2.42.0.166.g68748eb9c8
^ permalink raw reply related
* [PATCH v2 4/8] builtin/repack.c: extract redundant pack cleanup for existing packs
From: Taylor Blau @ 2023-09-13 19:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
To remove redundant packs at the end of a repacking operation, Git uses
its `remove_redundant_pack()` function in a loop over the set of
pre-existing, non-kept packs.
In a later commit, we will split this list into two, one for
pre-existing cruft pack(s), and another for non-cruft pack(s). Prepare
for this by factoring out the routine to loop over and delete redundant
packs into its own function.
Instead of calling `remove_redundant_pack()` directly, we now will call
`remove_redundant_existing_packs()`, which itself dispatches a call to
`remove_redundant_packs_1()`. Note that the geometric repacking code
will still call `remove_redundant_pack()` directly, but see the previous
commit for more details.
Having `remove_redundant_packs_1()` exist as a separate function may
seem like overkill in this patch. However, a later patch will call
`remove_redundant_packs_1()` once over two separate lists, so this
refactoring sets us up for that.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 45 ++++++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 17 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 71366811e9..b5fb14c017 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -135,6 +135,33 @@ static void mark_packs_for_deletion(struct existing_packs *existing,
mark_packs_for_deletion_1(names, &existing->non_kept_packs);
}
+static void remove_redundant_pack(const char *dir_name, const char *base_name)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct multi_pack_index *m = get_local_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);
+}
+
+static void remove_redundant_packs_1(struct string_list *packs)
+{
+ struct string_list_item *item;
+ for_each_string_list_item(item, packs) {
+ if (!((uintptr_t)item->util & DELETE_PACK))
+ continue;
+ remove_redundant_pack(packdir, item->string);
+ }
+}
+
+static void remove_redundant_existing_packs(struct existing_packs *existing)
+{
+ remove_redundant_packs_1(&existing->non_kept_packs);
+}
+
static void existing_packs_release(struct existing_packs *existing)
{
string_list_clear(&existing->kept_packs, 0);
@@ -184,18 +211,6 @@ static void collect_pack_filenames(struct existing_packs *existing,
strbuf_release(&buf);
}
-static void remove_redundant_pack(const char *dir_name, const char *base_name)
-{
- struct strbuf buf = STRBUF_INIT;
- struct multi_pack_index *m = get_local_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);
-}
-
static void prepare_pack_objects(struct child_process *cmd,
const struct pack_objects_args *args,
const char *out)
@@ -1222,11 +1237,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (delete_redundant) {
int opts = 0;
- for_each_string_list_item(item, &existing.non_kept_packs) {
- if (!((uintptr_t)item->util & DELETE_PACK))
- continue;
- remove_redundant_pack(packdir, item->string);
- }
+ remove_redundant_existing_packs(&existing);
if (geometry.split_factor)
geometry_remove_redundant_packs(&geometry, &names,
--
2.42.0.166.g68748eb9c8
^ permalink raw reply related
* [PATCH v2 5/8] builtin/repack.c: extract `has_existing_non_kept_packs()`
From: Taylor Blau @ 2023-09-13 19:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
When there is:
- at least one pre-existing packfile (which is not marked as kept),
- repacking with the `-d` flag, and
- not doing a cruft repack
, then we pass a handful of additional options to the inner
`pack-objects` process, like `--unpack-unreachable`,
`--keep-unreachable`, and `--pack-loose-unreachable`, in addition to
marking any packs we just wrote for promisor remotes as kept in-core
(with `--keep-pack`, as opposed to the presence of a ".keep" file on
disk).
Because we store both cruft and non-cruft packs together in the same
`existing.non_kept_packs` list, it suffices to check its `nr` member to
see if it is zero or not.
But a following change will store cruft- and non-cruft packs separately,
meaning this check would break as a result. Prepare for this by
extracting this part of the check into a new helper function called
`has_existing_non_kept_packs()`.
This patch does not introduce any functional changes, but prepares us to
make a more isolated change in a subsequent patch.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index b5fb14c017..9ebc2e774b 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -105,6 +105,11 @@ struct existing_packs {
.non_kept_packs = STRING_LIST_INIT_DUP, \
}
+static int has_existing_non_kept_packs(const struct existing_packs *existing)
+{
+ return existing->non_kept_packs.nr;
+}
+
static void mark_packs_for_deletion_1(struct string_list *names,
struct string_list *list)
{
@@ -1047,7 +1052,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
if (pack_everything & ALL_INTO_ONE) {
repack_promisor_objects(&po_args, &names);
- if (existing.non_kept_packs.nr && delete_redundant &&
+ if (has_existing_non_kept_packs(&existing) &&
+ delete_redundant &&
!(pack_everything & PACK_CRUFT)) {
for_each_string_list_item(item, &names) {
strvec_pushf(&cmd.args, "--keep-pack=%s-%s.pack",
--
2.42.0.166.g68748eb9c8
^ permalink raw reply related
* [PATCH v2 6/8] builtin/repack.c: store existing cruft packs separately
From: Taylor Blau @ 2023-09-13 19:17 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
When repacking with the `--write-midx` option, we invoke the function
`midx_included_packs()` in order to produce the list of packs we want to
include in the resulting MIDX.
This list is comprised of:
- existing .keep packs
- any pack(s) which were written earlier in the same process
- any unchanged packs when doing a `--geometric` repack
- any cruft packs
Prior to this patch, we stored pre-existing cruft and non-cruft packs
together (provided those packs are non-kept). This meant we needed an
additional bit to indicate which non-kept pack(s) were cruft versus
those that aren't.
But alternatively we can store cruft packs in a separate list, avoiding
the need for this extra bit, and simplifying the code below.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 9ebc2e774b..8103a9d308 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -27,7 +27,6 @@
#define PACK_CRUFT 4
#define DELETE_PACK 1
-#define CRUFT_PACK 2
static int pack_everything;
static int delta_base_offset = 1;
@@ -98,16 +97,18 @@ static int repack_config(const char *var, const char *value,
struct existing_packs {
struct string_list kept_packs;
struct string_list non_kept_packs;
+ struct string_list cruft_packs;
};
#define EXISTING_PACKS_INIT { \
.kept_packs = STRING_LIST_INIT_DUP, \
.non_kept_packs = STRING_LIST_INIT_DUP, \
+ .cruft_packs = STRING_LIST_INIT_DUP, \
}
static int has_existing_non_kept_packs(const struct existing_packs *existing)
{
- return existing->non_kept_packs.nr;
+ return existing->non_kept_packs.nr || existing->cruft_packs.nr;
}
static void mark_packs_for_deletion_1(struct string_list *names,
@@ -138,6 +139,7 @@ static void mark_packs_for_deletion(struct existing_packs *existing,
{
mark_packs_for_deletion_1(names, &existing->non_kept_packs);
+ mark_packs_for_deletion_1(names, &existing->cruft_packs);
}
static void remove_redundant_pack(const char *dir_name, const char *base_name)
@@ -165,12 +167,14 @@ static void remove_redundant_packs_1(struct string_list *packs)
static void remove_redundant_existing_packs(struct existing_packs *existing)
{
remove_redundant_packs_1(&existing->non_kept_packs);
+ remove_redundant_packs_1(&existing->cruft_packs);
}
static void existing_packs_release(struct existing_packs *existing)
{
string_list_clear(&existing->kept_packs, 0);
string_list_clear(&existing->non_kept_packs, 0);
+ string_list_clear(&existing->cruft_packs, 0);
}
/*
@@ -204,12 +208,10 @@ static void collect_pack_filenames(struct existing_packs *existing,
if ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)
string_list_append(&existing->kept_packs, buf.buf);
- else {
- struct string_list_item *item;
- item = string_list_append(&existing->non_kept_packs, buf.buf);
- if (p->is_cruft)
- item->util = (void*)(uintptr_t)CRUFT_PACK;
- }
+ else if (p->is_cruft)
+ string_list_append(&existing->cruft_packs, buf.buf);
+ else
+ string_list_append(&existing->non_kept_packs, buf.buf);
}
string_list_sort(&existing->kept_packs);
@@ -691,14 +693,11 @@ static void midx_included_packs(struct string_list *include,
string_list_insert(include, strbuf_detach(&buf, NULL));
}
- for_each_string_list_item(item, &existing->non_kept_packs) {
- if (!((uintptr_t)item->util & CRUFT_PACK)) {
- /*
- * no need to check DELETE_PACK, since we're not
- * doing an ALL_INTO_ONE repack
- */
- continue;
- }
+ for_each_string_list_item(item, &existing->cruft_packs) {
+ /*
+ * no need to check DELETE_PACK, since we're not
+ * doing an ALL_INTO_ONE repack
+ */
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
} else {
@@ -707,6 +706,12 @@ static void midx_included_packs(struct string_list *include,
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
+
+ for_each_string_list_item(item, &existing->cruft_packs) {
+ if ((uintptr_t)item->util & DELETE_PACK)
+ continue;
+ string_list_insert(include, xstrfmt("%s.idx", item->string));
+ }
}
}
@@ -835,6 +840,8 @@ static int write_cruft_pack(const struct pack_objects_args *args,
fprintf(in, "%s-%s.pack\n", pack_prefix, item->string);
for_each_string_list_item(item, &existing->non_kept_packs)
fprintf(in, "-%s.pack\n", item->string);
+ for_each_string_list_item(item, &existing->cruft_packs)
+ fprintf(in, "-%s.pack\n", item->string);
for_each_string_list_item(item, &existing->kept_packs)
fprintf(in, "%s.pack\n", item->string);
fclose(in);
--
2.42.0.166.g68748eb9c8
^ permalink raw reply related
* [PATCH v2 7/8] builtin/repack.c: avoid directly inspecting "util"
From: Taylor Blau @ 2023-09-13 19:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
The `->util` field corresponding to each string_list_item is used to
track the existence of some pack at the beginning of a repack operation
was originally intended to be used as a bitfield.
This bitfield tracked:
- (1 << 0): whether or not the pack should be deleted
- (1 << 1): whether or not the pack is cruft
The previous commit removed the use of the second bit, but a future
patch (from a different series than this one) will introduce a new use
of it.
So we could stop treating the util pointer as a bitfield and instead
start treating it as if it were a boolean. But this would require some
backtracking when that later patch is applied.
Instead, let's avoid touching the ->util field directly, and instead
introduce convenience functions like:
- pack_mark_for_deletion()
- pack_is_marked_for_deletion()
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 8103a9d308..e9a091fbbc 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -111,6 +111,16 @@ static int has_existing_non_kept_packs(const struct existing_packs *existing)
return existing->non_kept_packs.nr || existing->cruft_packs.nr;
}
+static void pack_mark_for_deletion(struct string_list_item *item)
+{
+ item->util = (void*)((uintptr_t)item->util | DELETE_PACK);
+}
+
+static int pack_is_marked_for_deletion(struct string_list_item *item)
+{
+ return (uintptr_t)item->util & DELETE_PACK;
+}
+
static void mark_packs_for_deletion_1(struct string_list *names,
struct string_list *list)
{
@@ -130,7 +140,7 @@ static void mark_packs_for_deletion_1(struct string_list *names,
* (if `-d` was given).
*/
if (!string_list_has_string(names, sha1))
- item->util = (void*)(uintptr_t)((size_t)item->util | DELETE_PACK);
+ pack_mark_for_deletion(item);
}
}
@@ -158,7 +168,7 @@ static void remove_redundant_packs_1(struct string_list *packs)
{
struct string_list_item *item;
for_each_string_list_item(item, packs) {
- if (!((uintptr_t)item->util & DELETE_PACK))
+ if (!pack_is_marked_for_deletion(item))
continue;
remove_redundant_pack(packdir, item->string);
}
@@ -702,13 +712,13 @@ static void midx_included_packs(struct string_list *include,
}
} else {
for_each_string_list_item(item, &existing->non_kept_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
+ if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
for_each_string_list_item(item, &existing->cruft_packs) {
- if ((uintptr_t)item->util & DELETE_PACK)
+ if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
--
2.42.0.166.g68748eb9c8
^ permalink raw reply related
* [PATCH v2 8/8] builtin/repack.c: extract common cruft pack loop
From: Taylor Blau @ 2023-09-13 19:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Patrick Steinhardt
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
When generating the list of packs to store in a MIDX (when given the
`--write-midx` option), we include any cruft packs both during
--geometric and non-geometric repacks.
But the rules for when we do and don't have to check whether any of
those cruft packs were queued for deletion differ slightly between the
two cases.
But the two can be unified, provided there is a little bit of extra
detail added in the comment to clarify when it is safe to avoid checking
for any pending deletions (and why it is OK to do so even when not
required).
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index e9a091fbbc..529e13120d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -702,26 +702,31 @@ static void midx_included_packs(struct string_list *include,
string_list_insert(include, strbuf_detach(&buf, NULL));
}
-
- for_each_string_list_item(item, &existing->cruft_packs) {
- /*
- * no need to check DELETE_PACK, since we're not
- * doing an ALL_INTO_ONE repack
- */
- string_list_insert(include, xstrfmt("%s.idx", item->string));
- }
} else {
for_each_string_list_item(item, &existing->non_kept_packs) {
if (pack_is_marked_for_deletion(item))
continue;
string_list_insert(include, xstrfmt("%s.idx", item->string));
}
+ }
- for_each_string_list_item(item, &existing->cruft_packs) {
- if (pack_is_marked_for_deletion(item))
- continue;
- string_list_insert(include, xstrfmt("%s.idx", item->string));
- }
+ for_each_string_list_item(item, &existing->cruft_packs) {
+ /*
+ * When doing a --geometric repack, there is no need to check
+ * for deleted packs, since we're by definition not doing an
+ * ALL_INTO_ONE repack (hence no packs will be deleted).
+ * Otherwise we must check for and exclude any packs which are
+ * enqueued for deletion.
+ *
+ * So we could omit the conditional below in the --geometric
+ * case, but doing so is unnecessary since no packs are marked
+ * as pending deletion (since we only call
+ * `mark_packs_for_deletion()` when doing an all-into-one
+ * repack).
+ */
+ if (pack_is_marked_for_deletion(item))
+ continue;
+ string_list_insert(include, xstrfmt("%s.idx", item->string));
}
}
--
2.42.0.166.g68748eb9c8
^ permalink raw reply related
* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Junio C Hamano @ 2023-09-13 19:44 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Jeff King, Patrick Steinhardt, Christian Couder
In-Reply-To: <cover.1694632644.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
> Here is a small reroll of my series to clean up some of the internals of
> 'git repack' used to track the set of existing packs.
>
> Much is unchanged from the last round, save for some additional clean-up
> on how we handle the '->util' field for each pack's string_list_item in
> response to very helpful review from those CC'd.
The change to [7/8] was as expected and looking good. Let's see if
we see additional reviews from others, plan to declare victory and
merge it to 'next' by early next week at the latest, if not sooner.
Christian, your cc/repack-sift-filtered-objects-to-separate-pack
topic will have to interact with this topic when merged to 'seen',
so it would be good for you to give a review on these patches (if
only to understand the new world order) and optionally make a trial
merge between the two to see how well they work together and what
adjustment will be needed when you eventually rebase your topic on
top. Actual rebasing can wait until this topic graduates, but trial
merge is something you can immediately do in the meantime to prepare
for the future.
Thanks.
^ permalink raw reply
* [BUG] `git push` sends unnecessary objects
From: Javier Mora @ 2023-09-13 22:59 UTC (permalink / raw)
To: git
I came across this issue accidentally when trying to move a directory
containing a very large file, and deleting another file in that
directory while I was at it.
It seems to be caused by `pack.useSparse=true` being the default since
v2.27 (which I found out after spending quite a while manually
bisecting and compiling git since I noticed that this didn't happen in
v2.25; commit de3a864 introduces this regression).
* Expected:
Pushing a commit that moves a file without modifying it shouldn't
require sending a blob object for that file, since the remote server
already has that blob object.
* Observed:
Pushing a commit that moves a directory containing a file and also
adds/deletes other files in that directory will for some reason also
send blobs for all the files in that directory, even the ones that
were already in the remote.
* Consequences:
This has a very big impact in push times for very small commits
that just move around files, if those files are very big (I had this
happen with a >100MB file over a problematic connection... yikes!)
* Note:
The commit introducing the regression does warn about possible
scenarios involving a special arrangement of exact copies across
directories, but these are not "copies", I just moved a file, which
seems like a rather common operation.
Code snippet for reproduction:
```
mkdir TEST_git
cd TEST_git
mkdir -p local remote/origin.git
cd remote/origin.git
git init --bare
cd ../../local
git init
git remote add origin file://"${PWD%/*}"/remote/origin.git
mkdir zig
for i in a b c d e; do
dd if=/dev/urandom of=zig/"$i" bs=1M count=1
done
git add .
git commit -m 'Add big files'
git push -u origin master
#>> Writing objects: 100% (8/8), 5.00 MiB | 13.27 MiB/s, done.
#^ makes sense: 1 commit + 2 trees (/ and /zig) + 5 files = 8;
# 5 MiB in total for the 5x 1 MiB binary files
git mv zig zag
git commit -m 'Move zig'
git push
#>> Writing objects: 100% (2/2), 233 bytes | 233.00 KiB/s, done.
#^ makes sense: 1 commit + 1 tree (/ renames /zig to /zag) = 2;
# a,b,c,d,e objects already in remote
git mv zag zog
touch zog/f
git add zog/f
git commit -m 'For great justice'
git push
#>> Writing objects: 100% (9/9), 5.00 MiB | 24.63 MiB/s, done.
#^ It re-uploaded the 5x 1 MiB blobs
# even though remote already had them.
```
Note that the latter doesn't happen if I use `git -c pack.useSparse=false push`.
^ permalink raw reply
* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Jeff King @ 2023-09-14 0:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git, Patrick Steinhardt, Christian Couder
In-Reply-To: <xmqqa5tpluyj.fsf@gitster.g>
On Wed, Sep 13, 2023 at 12:44:04PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Here is a small reroll of my series to clean up some of the internals of
> > 'git repack' used to track the set of existing packs.
> >
> > Much is unchanged from the last round, save for some additional clean-up
> > on how we handle the '->util' field for each pack's string_list_item in
> > response to very helpful review from those CC'd.
>
> The change to [7/8] was as expected and looking good. Let's see if
> we see additional reviews from others, plan to declare victory and
> merge it to 'next' by early next week at the latest, if not sooner.
This looks great to me. The motivation in the revised patch 7 is much
easier to follow, and the end result is much nicer to read. :)
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Jeff King @ 2023-09-14 0:30 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Johannes Schindelin
In-Reply-To: <59b423e9-d99e-4817-8a33-c50419593740@gmail.com>
On Wed, Sep 13, 2023 at 04:16:48PM +0100, Phillip Wood wrote:
> On 11/09/2023 10:36, Jeff King wrote:
> > On Thu, Sep 07, 2023 at 11:04:33AM +0100, Phillip Wood wrote:
> > Looking at the ci-config branch of phillipwood/git.git, I see this in
> > your allow-refs:
> >
> > refs/heads/main|refs/heads/master|refs/heads/maint|refs/heads/next|refs/heads/seen|refs/tags/gitgui-*|refs/tags/pr-[0-9]*|refs/tags/v[1-9].*)
> >
> > So you do use multiple prefixes, though all in refs/tags/. Do you
> > actually push tags for which you do want to run CI,
>
> Yes, but not very often - I could probably just reject all tags and start
> the CI manually when I want it (assuming that's an option). Thanks for
> digging into the various options, it sounds like it is possible so long as
> we don't want multiple prefixes.
I'll additionally have to switch to using the full refname in the
matches, but that is probably a reasonable thing to do anyway (it makes
config a bit more verbose, but it is obviously much more flexible).
We can do the loop-unroll thing if we really want to support multiple
prefixes, but if you're OK with it, let's try the single-prefix way and
see if anybody runs into problems (I'm still convinced there's only a
few of us using this stuff anyway). I'm hesitant to do the unroll just
because it requires picking a maximum value, with a bizarre failure if
you happen to have 4 prefixes or whatever.
I'll see if I can polish up what I showed earlier into a patch.
(BTW, one other thing I tried was using fromJSON(vars.CI_CONFIG) in the
"on" expression, which in theory would allow globbing. But it doesn't
look like expressions work at all in that context. And even if they did,
I'm not sure we could make it work such that an empty CI_CONFIG used
some defaults, since there's no conditional-expression ternary
operator).
> Aside: what I'd really like is to be able to set an environment variable
> when I push to skip or force the CI
>
> GITHUB_SKIP_CI=1 git push github ...
>
> but that would require support from the git client, the protocol and the
> server.
We have the necessary bits at the protocol: push-options. But it's up to
the server-side hooks to decide which ones are meaningful and to do
something useful with them. It looks like GitLab supports:
git push -o ci.skip=1 ...
but I don't think GitHub respects any equivalent option.
-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