* 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
* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Jeff King @ 2023-09-14 0:44 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Johannes Schindelin
In-Reply-To: <20230914003010.GA1709842@coredump.intra.peff.net>
Two minor corrections in what I wrote...
On Wed, Sep 13, 2023 at 08:30:10PM -0400, Jeff King wrote:
> (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).
It's not a real ?: operator, but GitHub does use the word "ternary" to
describe the short-circuit behavior of:
condition && "if-true" || "if-not-true"
So in theory we could do:
on: ${{ fromJSON(var.CI_ON != '' && vars.CI_ON || "[pull_request, push]" }}
but I couldn't make it work (it complains about the value of "on" in
such a way that I assume it is simply not expanding expressions at all).
> 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.
The syntax here is actually just "git push -o ci.skip", without the
equals.
-Peff
^ permalink raw reply
* Re: [PATCH v2 6/6] trailer: use offsets for trailer_start/trailer_end
From: Linus Arver @ 2023-09-14 1:21 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood
In-Reply-To: <xmqqmsxsv8ik.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Linus Arver <linusa@google.com>
>>
>> Previously these fields in the trailer_info struct were of type "const
>> char *" and pointed to positions in the input string directly (to the
>> start and end positions of the trailer block).
>>
>> Use offsets to make the intended usage less ambiguous. We only need to
>> reference the input string in format_trailer_info(), so update that
>> function to take a pointer to the input.
>
> Hmm, I am not sure if this is an improvement. If the underlying
> buffer can be reallocated (to grow), the approach to use the offsets
> certainly is easier to deal with, as they will stay valid even after
> such a reallocation. But you lose the obvious sentinel value NULL
> that can mean something special
True.
> and have to make the readers aware
> of the local convention you happened to have picked with a comment
> like ...
>
>> Signed-off-by: Linus Arver <linusa@google.com>
>> ---
>> trailer.c | 17 ++++++++---------
>> trailer.h | 7 +++----
>> 2 files changed, 11 insertions(+), 13 deletions(-)
>> ...
>> /*
>> - * Pointers to the start and end of the trailer block found. If there
>> - * is no trailer block found, these 2 pointers point to the end of the
>> - * input string.
>> + * Offsets to the trailer block start and end positions in the input
>> + * string. If no trailer block is found, these are set to 0.
>> */
>
> ... this, simply because there is no obvious sentinel value for an
> unsigned integral type; even if you count MAX_ULONG and its friends,
> they are not as obvious as NULL for pointer types.
I agree that there is no trustworthy sentinel value for an unsigned
integral type.
On the other hand, we never used NULL as a sentinel value before even
when they were const char pointers --- the current comment for these
fields which say ...
If there is no trailer block found, these 2 pointers point to the end of the
input string.
... sounds somewhat arbitrary to me (and I don't think we care about
this property in trailer.c, and AFAICS it's also not something that
consumers should be aware of). Consumers of the trailer_info struct
could also just see if "info->trailer_nr > 0" to check whether any
trailers were found, although if we're merging Patch 1 [1] of this
series the consumers will not have easy access any more to any of the
trailer_info fields, and they should instead be using a public-facing
function that does the "were trailers found" check.
> So, I dunno.
If the "we don't use NULL sentinel values currently anyway" argument is
convincing enough, I'm happy to add this to the commit message on a
reroll. But I'm also OK with dropping this patch. Thoughts?
[1] https://lore.kernel.org/git/pull.1563.git.1691211879.gitgitgadget@gmail.com/T/#m8f1b5f1eb346331658c8c7b3e057a4ee31223664
^ permalink raw reply
* Re: [PATCH v2 4/6] trailer: teach find_patch_start about --no-divider
From: Linus Arver @ 2023-09-14 2:19 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood
In-Reply-To: <xmqqv8cgvd02.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Linus Arver <linusa@google.com>
>>
>> Currently, find_patch_start only finds the start of the patch part of
>> the input (by looking at the "---" divider) for cases where the
>> "--no-divider" flag has not been provided. If the user provides this
>> flag, we do not rely on find_patch_start at all and just call strlen()
>> directly on the input.
>>
>> Instead, make find_patch_start aware of "--no-divider" and make it
>> handle that case as well. This means we no longer need to call strlen at
>> all and can just rely on the existing code in find_patch_start. By
>> forcing callers to consider this important option, we avoid the kind of
>> mistake described in be3d654343 (commit: pass --no-divider to
>> interpret-trailers, 2023-06-17).
>
> OK. The code pays attention to "---" so making it stop doing so
> when the "--no-*" option is given will make the function responsible
> for finding the beginning of the patch.
>
> I wonder if we should rename this function while we are at it,
> though. When "--no-divider" is given, the expected use case is
> *not* to have a patch at all, and it is dubious that a function
> whose name is find_patch_start() can possibly do anything useful.
IOW, saying as the caller of this function, "find the patch start in
this input I'm giving you, but also FYI the input has no patch in it"
sounds wrong. Agreed.
> The real purpose of this function is to find the end of the log
> message, isn't it?
Indeed.
> And the caller uses the end of the log message
> it found and gives it to find_trailer_start() and find_trailer_end()
> as the upper boundary of the search for the trailer block.
Right! So a better name might be something like
"find_trailer_search_boundary" with a comment like
Find the point at which we should stop searching for trailers (to
parse them). This is either the end of the input string (obviously),
or the point when we see "---" indicating the start of the "patch
part".
Will update.
^ permalink raw reply
* Re: [PATCH v2 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED
From: Linus Arver @ 2023-09-14 2:41 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Christian Couder, Phillip Wood
In-Reply-To: <xmqqr0n4v8ul.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Linus Arver <linusa@google.com>
>>
>> [...]
>> So instead of using "*_DEFAULT", use "*_UNSPECIFIED" because this
>> signals to the reader that the *_UNSPECIFIED value by itself carries no
>> meaning (it's a zero value and by itself does not "default" to anything,
>> necessitating the need to have some other way of getting to a useful
>> value).
>
> It gets tempting to initialize a variable to the default and arrange
> the rest of the system so that the variable set to the default
> triggers the default activity. Such an obvious solution however
> cannot be used when (1) being left unspecified to use the default
> value and (2) explicitly set by the user to a value that happens to
> be the same as the default have to behave differently. I am not
> sure if that applies to the trailers system, though.
>
> Thanks.
I get the feeling that you wrote the "Such an obvious ... differently"
sentence after writing the last sentence in that paragraph, because when
you say
I am not
sure if that applies to the trailers system, though.
I read the "that" (emphasis added) in there as referring to the solution
described in the first sentence, and not the conditions (1) and (2) you
enumerated. IOW, you are OK with this patch.
Am I parsing your paragraph correctly?
> PS. Glen's old e-mail address is no longer valid and there is no
> forwarding done by @google.com mailservers, it seems. Can you tell
> GGG to drop the address (optionally replace it with his new address)?
Thanks for catching this. I've updated the GGG PR [1] to use Glen's new
address.
[1] https://github.com/gitgitgadget/git/pull/1563#issue-1837440424
^ permalink raw reply
* Re: [PATCH v2 4/6] trailer: teach find_patch_start about --no-divider
From: Junio C Hamano @ 2023-09-14 3:12 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Glen Choo, Christian Couder,
Phillip Wood
In-Reply-To: <owly34zhlcoa.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
>> The real purpose of this function is to find the end of the log
>> message, isn't it?
>
> Indeed.
> ...
> Right! So a better name might be something like
> "find_trailer_search_boundary" with a comment like
Or "find_end_of_log_message()", which we agreed to be the real
purpose of this function ;-)
^ permalink raw reply
* Re: [PATCH v2 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED
From: Junio C Hamano @ 2023-09-14 3:16 UTC (permalink / raw)
To: Linus Arver
Cc: Linus Arver via GitGitGadget, git, Christian Couder, Phillip Wood
In-Reply-To: <owlyzg1pjx2f.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
>> It gets tempting to initialize a variable to the default and arrange
>> the rest of the system so that the variable set to the default
>> triggers the default activity. Such an obvious solution however
>> cannot be used when (1) being left unspecified to use the default
>> value and (2) explicitly set by the user to a value that happens to
>> be the same as the default have to behave differently. I am not
>> sure if that applies to the trailers system, though.
>>
>> Thanks.
>
> I get the feeling that you wrote the "Such an obvious ... differently"
> sentence after writing the last sentence in that paragraph, because when
> you say
>
> I am not
> sure if that applies to the trailers system, though.
>
> I read the "that" (emphasis added) in there as referring to the solution
> described in the first sentence, and not the conditions (1) and (2) you
> enumerated. IOW, you are OK with this patch.
"that" refers to "the reason not to use such an obvious solution".
I do not know if trailer subsystem wants to treat "left unspecified"
and "set to the value that happens to be the same as the default" in
a different way. If it does want to do so, then I do not see a
strong reason not to use the "obvious solution".
Thanks.
^ permalink raw reply
* Re: [PATCH v2 6/6] trailer: use offsets for trailer_start/trailer_end
From: Linus Arver @ 2023-09-14 3:18 UTC (permalink / raw)
To: Junio C Hamano, Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood
In-Reply-To: <owly5y4dlfcg.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Linus Arver <linusa@google.com>
>>>
>>> Previously these fields in the trailer_info struct were of type "const
>>> char *" and pointed to positions in the input string directly (to the
>>> start and end positions of the trailer block).
>>>
>>> Use offsets to make the intended usage less ambiguous. We only need to
>>> reference the input string in format_trailer_info(), so update that
>>> function to take a pointer to the input.
>>
>> Hmm, I am not sure if this is an improvement. If the underlying
>> buffer can be reallocated (to grow), the approach to use the offsets
>> certainly is easier to deal with, as they will stay valid even after
>> such a reallocation. But you lose the obvious sentinel value NULL
>> that can mean something special
>
> True.
>
>> and have to make the readers aware
>> of the local convention you happened to have picked with a comment
>> like ...
>>
>>> Signed-off-by: Linus Arver <linusa@google.com>
>>> ---
>>> trailer.c | 17 ++++++++---------
>>> trailer.h | 7 +++----
>>> 2 files changed, 11 insertions(+), 13 deletions(-)
>>> ...
>>> /*
>>> - * Pointers to the start and end of the trailer block found. If there
>>> - * is no trailer block found, these 2 pointers point to the end of the
>>> - * input string.
>>> + * Offsets to the trailer block start and end positions in the input
>>> + * string. If no trailer block is found, these are set to 0.
>>> */
I've just realized that the new comment "If no trailer block is found,
these are set to 0" is perhaps not always true in the current version
of this patch. This is because this patch did a mechanical type
conversion of the old fields from "const char *" to "size_t", and we
still run
trailer_end = find_trailer_end(str, trailer_search_boundary);
trailer_start = find_trailer_start(str, trailer_end);
inside trailer_info_get() (not visible in the patch context lines). I
will update this patch to make the comment "If no trailer block is found,
these are set to 0" true.
^ permalink raw reply
* [PATCH] doc: pull: improve rebase=false documentation
From: Dragan Simic @ 2023-09-14 3:25 UTC (permalink / raw)
To: git
Mention in the documentation that --rebase defaults to false.
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
Documentation/git-pull.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 0e14f8b5b2..d28790388e 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -116,7 +116,8 @@ When set to `merges`, rebase using `git rebase --rebase-merges` so that
the local merge commits are included in the rebase (see
linkgit:git-rebase[1] for details).
+
-When false, merge the upstream branch into the current branch.
+When false, merge the upstream branch into the current branch. This is
+the default.
+
When `interactive`, enable the interactive mode of rebase.
+
^ permalink raw reply related
* Re: [PATCH v2 4/6] trailer: teach find_patch_start about --no-divider
From: Linus Arver @ 2023-09-14 5:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Glen Choo, Christian Couder,
Phillip Wood
In-Reply-To: <xmqq5y4dla6p.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>>> The real purpose of this function is to find the end of the log
>>> message, isn't it?
>>
>> Indeed.
>> ...
>> Right! So a better name might be something like
>> "find_trailer_search_boundary" with a comment like
>
> Or "find_end_of_log_message()", which we agreed to be the real
> purpose of this function ;-)
I did this locally, but in doing so realized that we have in trailer.c
/* Return the position of the end of the trailers. */
static size_t find_trailer_end(const char *buf, size_t len)
{
return len - ignore_non_trailer(buf, len);
}
and the ignore_non_trailer() comes from commit.c, which says
/*
* Inspect the given string and determine the true "end" of the log message, in
* order to find where to put a new Signed-off-by trailer. Ignored are
* trailing comment lines and blank lines. To support "git commit -s
* --amend" on an existing commit, we also ignore "Conflicts:". To
* support "git commit -v", we truncate at cut lines.
*
* Returns the number of bytes from the tail to ignore, to be fed as
* the second parameter to append_signoff().
*/
size_t ignore_non_trailer(const char *buf, size_t len)
{
...and I am not so sure the new "find_end_of_log_message" name for
find_patch_start() is desirable because of the overlap in meaning with
the comment for ignore_non_trailer(). To recap, we have in
trailer_info_get() in trailer.c which (without this patch) has
if (opts->no_divider)
patch_start = strlen(str);
else
patch_start = find_patch_start(str);
trailer_end = find_trailer_end(str, patch_start);
trailer_start = find_trailer_start(str, trailer_end);
to find the trailer_end and trailer_start positions in the input. These
positions are the boundaries for parsing for actual trailers (if any).
More precisely, the "patch_start" variable helps us _skip_ the "patch
part" of the input (if any, denoted by "---"). The call to
find_trailer_end() helps us (again) _skip_ any parts that are not part
of the actual log message (per the comment in ignore_non_trailer()). So
as far as trailer_info_get() is concerned, we are just trying to skip
over areas where we shouldn't search/parse for trailers.
The above analysis leads me to some new ideas:
(1) For "find_end_of_log_message()", I think this name should really
belong to ignore_non_trailer() in commit.c instead of
find_patch_start() in trailer.c. The existing comment for
ignore_non_trailer() already says that its job is to determine the
true end of the log message (and does a lot of the necessary work to
do this job).
(2) find_patch_start() should be named something like
"shrink_trailer_search_space" (although this meaning belongs equally
well to find_trailer_end()), because the point is to reduce the
search space for parsing trailers.
(3) "find_trailer_end" is not a great function name because on first
glance it implies that it found the end of trailers, but this is
only true for the "happy path" of actually finding trailers.
I need to consider all of the above ideas and reroll this patch. Because
the ideas here closely relate to the "trailer_end" and "trailer_start"
variables, I will probably reorder the series so that this patch and the
last patch are closer together.
^ permalink raw reply
* Re: [PATCH v3 0/3] "update-index --show-index-version"
From: Linus Arver @ 2023-09-14 6:19 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20230912193235.776292-1-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
>
> This iteration takes suggestions by Linus Arver; the tests added by
> [2/3] have been clarified.
>
This version LGTM, thanks!
^ permalink raw reply
* Re: [PATCH v3 1/1] range-diff: treat notes like `log`
From: Johannes Schindelin @ 2023-09-14 8:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, git, Denton Liu, Jeff King
In-Reply-To: <xmqqzg1strgx.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 4717 bytes --]
Hi Junio,
On Mon, 11 Sep 2023, Junio C Hamano wrote:
> Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>
> >> To fix this explicitly set the output format of the internally executed
> >> `git log` with `--pretty=medium`. Because that cancels `--notes`, add
> >> explicitly `--notes` at the end.
> >
> > § Authors
> >
> > • Fix by Johannes
> > • Tests by Kristoffer
> >
> > † 1: See e.g. 66b2ed09c2 (Fix "log" family not to be too agressive about
> > showing notes, 2010-01-20).
> >
> > Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> > ---
>
> OK, Dscho, does this round look acceptable to you?
>
> It feels UGLY to iterate over args _without_ actually parsing them,
> at least to me. Such a non-parsing look breaks at least in two ways
> over time. (1) a mechanism may be introduced laster, similar to
> "--", that allows other_arg->v[] array to mark "here is where the
> dashed options end". Now the existing loop keeps reading to the end
> and finds "--notes" that is not a dashed option but is part of the
> normal command line arguments in "other arg". (2) Among the dashed
> options passed in the other_arg->v[], there may be an option that
> takes a string value, and a value that happens to be "--notes" is
> mistaken as asking for "notes" (iow "git log -G --notes" is looking
> for commits with changes that contain "double dash followed by en oh
> tee ee es").
>
> I think "git range-diff -G --notes" (or "-S --notes") shows that
> this new non-parsing loop is already broken. It looks for a change
> that has "--notes" correctly, but at the same time, triggers that
> "ah, we have an explicit --notes so drop the implicit_notes_arg
> flag" logic.
Right, `-G --notes` is a good argument to rethink this.
A much more surgical way to address the issue at hand might be this
(Kristoffer, what do you think? It's missing documentation for the
newly-introduced `--show-notes-by-default` option, but you get the idea):
-- snipsnap --
diff --git a/range-diff.c b/range-diff.c
index fbb81a92cc0..56f6870ff91 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -41,20 +41,12 @@ static int read_patches(const char *range, struct string_list *list,
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT, contents = STRBUF_INIT;
struct patch_util *util = NULL;
- int i, implicit_notes_arg = 1, in_header = 1;
+ int in_header = 1;
char *line, *current_filename = NULL;
ssize_t len;
size_t size;
int ret = -1;
- for (i = 0; other_arg && i < other_arg->nr; i++)
- if (!strcmp(other_arg->v[i], "--notes") ||
- starts_with(other_arg->v[i], "--notes=") ||
- !strcmp(other_arg->v[i], "--no-notes")) {
- implicit_notes_arg = 0;
- break;
- }
-
strvec_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
"--reverse", "--date-order", "--decorate=no",
"--no-prefix", "--submodule=short",
@@ -68,9 +60,8 @@ static int read_patches(const char *range, struct string_list *list,
"--output-indicator-context=#",
"--no-abbrev-commit",
"--pretty=medium",
+ "--show-notes-by-default",
NULL);
- if (implicit_notes_arg)
- strvec_push(&cp.args, "--notes");
strvec_push(&cp.args, range);
if (other_arg)
strvec_pushv(&cp.args, other_arg->v);
diff --git a/revision.c b/revision.c
index 2f4c53ea207..49d385257ac 100644
--- a/revision.c
+++ b/revision.c
@@ -2484,6 +2484,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->break_bar = xstrdup(optarg);
revs->track_linear = 1;
revs->track_first_time = 1;
+ } else if (!strcmp(arg, "--show-notes-by-default")) {
+ revs->show_notes_by_default = 1;
} else if (skip_prefix(arg, "--show-notes=", &optarg) ||
skip_prefix(arg, "--notes=", &optarg)) {
if (starts_with(arg, "--show-notes=") &&
@@ -3054,6 +3056,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
if (revs->expand_tabs_in_log < 0)
revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
+ if (!revs->show_notes_given && revs->show_notes_by_default) {
+ enable_default_display_notes(&revs->notes_opt, &revs->show_notes);
+ revs->show_notes_given = 1;
+ }
+
return left;
}
diff --git a/revision.h b/revision.h
index 82ab400139d..50091bbd13f 100644
--- a/revision.h
+++ b/revision.h
@@ -253,6 +253,7 @@ struct rev_info {
shown_dashes:1,
show_merge:1,
show_notes_given:1,
+ show_notes_by_default:1,
show_signature:1,
pretty_given:1,
abbrev_commit:1,
^ permalink raw reply related
* [PATCH 0/4] merge-ort unused parameter cleanups
From: Jeff King @ 2023-09-14 9:34 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
A few small cleanups for merge-ort collected from playing with
-Wunused-parameter. The first one actually fixes a user-visible bug, and
the rest are just cleanups.
[1/4]: merge-ort: drop custom err() function
[2/4]: merge-ort: stop passing "opt" to read_oid_strbuf()
[3/4]: merge-ort: drop unused parameters from detect_and_process_renames()
[4/4]: merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
merge-ort.c | 48 +++++++++++--------------------------------
t/t6406-merge-attr.sh | 3 ++-
2 files changed, 14 insertions(+), 37 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 1/4] merge-ort: drop custom err() function
From: Jeff King @ 2023-09-14 9:39 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
In-Reply-To: <20230914093409.GA2254811@coredump.intra.peff.net>
The merge-ort code has an err() function, but it's really just error()
in disguise. It differs in two ways:
1. It takes a "struct merge_options" argument. But the function
completely ignores it! We can simply remove it.
2. It formats the error string into a strbuf, prepending "error: ",
and then feeds the result into error(). But this is wrong! The
error() function already adds the prefix, so we end up with:
error: error: Failed to execute internal merge
So let's just drop this function entirely and call error() directly, as
the functions are otherwise identical (note that they both always return
-1).
Presumably nobody noticed the bogus messages because they are quite hard
to trigger (they are mostly internal errors reading and writing
objects). However, one easy trigger is a custom merge driver which dies
by signal; we have a test already here, but we were not checking the
contents of stderr.
Signed-off-by: Jeff King <peff@peff.net>
---
A few of these messages starts with capital letters, which is unlike our
usual error message style. I didn't clean that up here. We could do so
on top, but I actually wonder if some of these ought to be using
path_msg() and continuing instead, to give output closer to other
conflict or error cases (e.g., conflicts caused by missing submodule
objects). But I dunno. I guess these are all more clearly "woah,
something is totally wrong" that we do not expect to happen, so it
probably isn't a big deal to just abort.
merge-ort.c | 28 +++++-----------------------
t/t6406-merge-attr.sh | 3 ++-
2 files changed, 7 insertions(+), 24 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 8631c99700..027ecc7f78 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -721,23 +721,6 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
renames->callback_data_nr = renames->callback_data_alloc = 0;
}
-__attribute__((format (printf, 2, 3)))
-static int err(struct merge_options *opt, const char *err, ...)
-{
- va_list params;
- struct strbuf sb = STRBUF_INIT;
-
- strbuf_addstr(&sb, "error: ");
- va_start(params, err);
- strbuf_vaddf(&sb, err, params);
- va_end(params);
-
- error("%s", sb.buf);
- strbuf_release(&sb);
-
- return -1;
-}
-
static void format_commit(struct strbuf *sb,
int indent,
struct repository *repo,
@@ -2122,13 +2105,12 @@ static int handle_content_merge(struct merge_options *opt,
&result_buf);
if ((merge_status < 0) || !result_buf.ptr)
- ret = err(opt, _("Failed to execute internal merge"));
+ ret = error(_("Failed to execute internal merge"));
if (!ret &&
write_object_file(result_buf.ptr, result_buf.size,
OBJ_BLOB, &result->oid))
- ret = err(opt, _("Unable to add %s to database"),
- path);
+ ret = error(_("Unable to add %s to database"), path);
free(result_buf.ptr);
if (ret)
@@ -3518,10 +3500,10 @@ static int read_oid_strbuf(struct merge_options *opt,
unsigned long size;
buf = repo_read_object_file(the_repository, oid, &type, &size);
if (!buf)
- return err(opt, _("cannot read object %s"), oid_to_hex(oid));
+ return error(_("cannot read object %s"), oid_to_hex(oid));
if (type != OBJ_BLOB) {
free(buf);
- return err(opt, _("object %s is not a blob"), oid_to_hex(oid));
+ return error(_("object %s is not a blob"), oid_to_hex(oid));
}
strbuf_attach(dst, buf, size, size + 1);
return 0;
@@ -4973,7 +4955,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
* TRANSLATORS: The %s arguments are: 1) tree hash of a merge
* base, and 2-3) the trees for the two trees we're merging.
*/
- err(opt, _("collecting merge info failed for trees %s, %s, %s"),
+ error(_("collecting merge info failed for trees %s, %s, %s"),
oid_to_hex(&merge_base->object.oid),
oid_to_hex(&side1->object.oid),
oid_to_hex(&side2->object.oid));
diff --git a/t/t6406-merge-attr.sh b/t/t6406-merge-attr.sh
index 9677180a5b..05ad13b23e 100755
--- a/t/t6406-merge-attr.sh
+++ b/t/t6406-merge-attr.sh
@@ -179,7 +179,8 @@ test_expect_success !WINDOWS 'custom merge driver that is killed with a signal'
>./please-abort &&
echo "* merge=custom" >.gitattributes &&
- test_must_fail git merge main &&
+ test_must_fail git merge main 2>err &&
+ grep "^error: Failed to execute internal merge" err &&
git ls-files -u >output &&
git diff --name-only HEAD >>output &&
test_must_be_empty output
--
2.42.0.628.g8a27295885
^ permalink raw reply related
* [PATCH 2/4] merge-ort: stop passing "opt" to read_oid_strbuf()
From: Jeff King @ 2023-09-14 9:39 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
In-Reply-To: <20230914093409.GA2254811@coredump.intra.peff.net>
This function doesn't look at its merge_options parameter. It used to
pass it down to err(), but that function no longer exists (and didn't
look at "opt" anyway). We can drop it here.
Signed-off-by: Jeff King <peff@peff.net>
---
merge-ort.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 027ecc7f78..31c663b297 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3491,8 +3491,7 @@ static int sort_dirs_next_to_their_children(const char *one, const char *two)
return c1 - c2;
}
-static int read_oid_strbuf(struct merge_options *opt,
- const struct object_id *oid,
+static int read_oid_strbuf(const struct object_id *oid,
struct strbuf *dst)
{
void *buf;
@@ -3527,8 +3526,8 @@ static int blob_unchanged(struct merge_options *opt,
if (oideq(&base->oid, &side->oid))
return 1;
- if (read_oid_strbuf(opt, &base->oid, &basebuf) ||
- read_oid_strbuf(opt, &side->oid, &sidebuf))
+ if (read_oid_strbuf(&base->oid, &basebuf) ||
+ read_oid_strbuf(&side->oid, &sidebuf))
goto error_return;
/*
* Note: binary | is used so that both renormalizations are
--
2.42.0.628.g8a27295885
^ permalink raw reply related
* [PATCH 3/4] merge-ort: drop unused parameters from detect_and_process_renames()
From: Jeff King @ 2023-09-14 9:39 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
In-Reply-To: <20230914093409.GA2254811@coredump.intra.peff.net>
This function takes three trees representing the merge base and both
sides of the merge, but never looks at any of them. This is due to
f78cf97617 (merge-ort: call diffcore_rename() directly, 2021-02-14).
Prior to that commit, we passed pairs of trees to diff_tree_oid(). But
after that commit, we collect a custom diff_queue for each pair in the
merge_options struct, and just run diffcore_rename() on the result. So
the function does not need to know about the original trees at all
anymore.
Signed-off-by: Jeff King <peff@peff.net>
---
merge-ort.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 31c663b297..20eefd9b5e 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3324,10 +3324,7 @@ static int collect_renames(struct merge_options *opt,
return clean;
}
-static int detect_and_process_renames(struct merge_options *opt,
- struct tree *merge_base,
- struct tree *side1,
- struct tree *side2)
+static int detect_and_process_renames(struct merge_options *opt)
{
struct diff_queue_struct combined = { 0 };
struct rename_info *renames = &opt->priv->renames;
@@ -4964,8 +4961,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
trace2_region_leave("merge", "collect_merge_info", opt->repo);
trace2_region_enter("merge", "renames", opt->repo);
- result->clean = detect_and_process_renames(opt, merge_base,
- side1, side2);
+ result->clean = detect_and_process_renames(opt);
trace2_region_leave("merge", "renames", opt->repo);
if (opt->priv->renames.redo_after_renames == 2) {
trace2_region_enter("merge", "reset_maps", opt->repo);
--
2.42.0.628.g8a27295885
^ permalink raw reply related
* [PATCH 4/4] merge-ort: drop unused "opt" parameter from merge_check_renames_reusable()
From: Jeff King @ 2023-09-14 9:40 UTC (permalink / raw)
To: git; +Cc: Elijah Newren
In-Reply-To: <20230914093409.GA2254811@coredump.intra.peff.net>
The merge_options parameter has never been used since the function was
introduced in 64aceb6d73 (merge-ort: add code to check for whether
cached renames can be reused, 2021-05-20). In theory some merge options
might impact our decisions here, but that has never been the case so
far.
Let's drop it to appease -Wunused-parameter; it would be easy to add
back later if we need to (there is only one caller).
Signed-off-by: Jeff King <peff@peff.net>
---
merge-ort.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 20eefd9b5e..3953c9f745 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4880,8 +4880,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
trace2_region_leave("merge", "allocate/init", opt->repo);
}
-static void merge_check_renames_reusable(struct merge_options *opt,
- struct merge_result *result,
+static void merge_check_renames_reusable(struct merge_result *result,
struct tree *merge_base,
struct tree *side1,
struct tree *side2)
@@ -5083,7 +5082,7 @@ void merge_incore_nonrecursive(struct merge_options *opt,
trace2_region_enter("merge", "merge_start", opt->repo);
assert(opt->ancestor != NULL);
- merge_check_renames_reusable(opt, result, merge_base, side1, side2);
+ merge_check_renames_reusable(result, merge_base, side1, side2);
merge_start(opt, result);
/*
* Record the trees used in this merge, so if there's a next merge in
--
2.42.0.628.g8a27295885
^ permalink raw reply related
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Jeff King @ 2023-09-14 9:56 UTC (permalink / raw)
To: phillip.wood
Cc: Phillip Wood via GitGitGadget, git, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <9ba22d4b-3cbe-4d4a-8dba-bc3781e82222@gmail.com>
On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:
> Do we want a whole new session? As I understand it to launch a foreground
> job shells put the child in its own process group and then call tcsetpgrp()
> to change the foreground process group of the controlling terminal to that
> of the child. I agree that would be a better way of doing things on unix.
One thing I am not clear on is the convention on who does the process
group and controlling terminal setup. Should Git do it to say "I am
handing off control of the terminal to the editor that I am spawning"?
Or should the editor say "I am an editor which has a user interface that
takes over the terminal; I will control it while I am running".
The latter makes much more sense to me, as Git cannot know how the
editor plans to behave. But as I understand it, this kind of job control
stuff is implemented by the calling shell, which does the tcsetpgrp()
call.
So I dunno. It sounds to me like the "right" thing here is making Git
more shell-like in handing control to a program (like the editor) that
we expect to be in the foreground of the terminal. As opposed to the
"ignore SIGINT temporarily" thing which feels more like band-aid. But
I'm wary of getting into a rabbit hole of portability headaches and
weird corners of Unix terminal-handling conventions.
-Peff
^ permalink raw reply
* skip-worktree autostash on pull
From: Blake Campbell @ 2023-09-14 9:54 UTC (permalink / raw)
To: git
Hi all - I use update-index --skip-worktree on some config files that I change locally and don’t want to commit, but every time I pull from the remote I have to go through a process of no-skip-worktree, stash, pull, stash pop, then skip-worktree again, which is all a bit tedious! Ideally some switch like --autostash for git pull would be really useful. Does anyone know if something like that exists?
Thanks!
Blake.
^ permalink raw reply
* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Patrick Steinhardt @ 2023-09-14 10:33 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Taylor Blau, git, Christian Couder
In-Reply-To: <20230914000750.GA1709736@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
On Wed, Sep 13, 2023 at 08:07:50PM -0400, Jeff King wrote:
> 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
Agreed, the patch series loogs good to me now. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Phillip Wood @ 2023-09-14 10:49 UTC (permalink / raw)
To: Jeff King, phillip.wood; +Cc: git, Johannes Schindelin
In-Reply-To: <20230914003010.GA1709842@coredump.intra.peff.net>
On 14/09/2023 01:30, Jeff King wrote:
> On Wed, Sep 13, 2023 at 04:16:48PM +0100, Phillip Wood wrote:
> 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.
Lets start with a single-prefix and see if anyone complains
>> 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 ...
Oh I didn't know about that
> but I don't think GitHub respects any equivalent option.
That's a shame
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Christian Couder @ 2023-09-14 11:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git, Jeff King, Patrick Steinhardt
In-Reply-To: <xmqqa5tpluyj.fsf@gitster.g>
On Wed, Sep 13, 2023 at 9:44 PM Junio C Hamano <gitster@pobox.com> 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.
>
> 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.
Ok, I will try to review and merge this with
cc/repack-sift-filtered-objects-to-separate-pack soon.
^ permalink raw reply
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Phillip Wood @ 2023-09-14 13:50 UTC (permalink / raw)
To: Jeff King, phillip.wood
Cc: Phillip Wood via GitGitGadget, git, Johannes Schindelin,
Junio C Hamano
In-Reply-To: <20230914095601.GE2254894@coredump.intra.peff.net>
On 14/09/2023 10:56, Jeff King wrote:
> On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:
>
>> Do we want a whole new session? As I understand it to launch a foreground
>> job shells put the child in its own process group and then call tcsetpgrp()
>> to change the foreground process group of the controlling terminal to that
>> of the child. I agree that would be a better way of doing things on unix.
>
> One thing I am not clear on is the convention on who does the process
> group and controlling terminal setup. Should Git do it to say "I am
> handing off control of the terminal to the editor that I am spawning"?
> Or should the editor say "I am an editor which has a user interface that
> takes over the terminal; I will control it while I am running".
As I understand it the editor has a controlling terminal (assuming there
is a controlling terminal associated with the editor's session id), not
the other way round. If the editor is launched in the background then it
will receive SIGTTIN when it tries to read from it's controlling
terminal which stops the process unless the process installs a signal
handler.
> The latter makes much more sense to me, as Git cannot know how the
> editor plans to behave. But as I understand it, this kind of job control
> stuff is implemented by the calling shell, which does the tcsetpgrp()
> call.
Yes, my understanding is that the shell puts all the processes in a
pipeline in the same process group and calls tcsetpgrp() if it wants
that job to be run in the foreground.
> So I dunno. It sounds to me like the "right" thing here is making Git
> more shell-like in handing control to a program (like the editor) that
> we expect to be in the foreground of the terminal. As opposed to the
> "ignore SIGINT temporarily" thing which feels more like band-aid. But
> I'm wary of getting into a rabbit hole of portability headaches and
> weird corners of Unix terminal-handling conventions.
I'm wary of that too, it has the potential to end up adding a lot of
fiddly code checking if git is in the foreground or background and
propagating SIGTSTP and friends up to the shell. In any case we'd need
the "ignore SIGINT" thing for windows anyway. Ignoring SIGINT and
SIGQUIT in the parent is what system(3) does. As long as git exits
promptly when the interrupted child is killed I think that is
reasonable. Would you be happier if we re-raised the signal once we have
cleaned up any state that needs to be written before exiting?
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3 1/1] range-diff: treat notes like `log`
From: Junio C Hamano @ 2023-09-14 16:18 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Kristoffer Haugsbakk, git, Denton Liu, Jeff King
In-Reply-To: <dd2958c5-58bf-86dd-b666-9033259a8e1a@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> A much more surgical way to address the issue at hand might be this
> (Kristoffer, what do you think? It's missing documentation for the
> newly-introduced `--show-notes-by-default` option, but you get the idea):
Clever. I was wondering if the option parsing done by the revision
machinery was readily available for us to use from range-diff code,
but because we are forking out to "log" anyway, giving it the new
"--show-notes-by-default" option does sound like a much easier way
out. It would help something like
[alias] ln = !"git log --show-notes-by-default"
as well. Nicely designed.
^ permalink raw reply
* Re: [PATCH v2 0/8] repack: refactor pack snapshot-ing logic
From: Taylor Blau @ 2023-09-14 18:01 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, git, Jeff King, Patrick Steinhardt
In-Reply-To: <CAP8UFD1rDb-iYf4LYb7n=K4KpQ-JR-JK4TkQpGJ-TCfTNFFbnA@mail.gmail.com>
On Thu, Sep 14, 2023 at 01:10:38PM +0200, Christian Couder wrote:
> Ok, I will try to review and merge this with
> cc/repack-sift-filtered-objects-to-separate-pack soon.
I took a look at how much/little effort was going to be required, and
luckily the changes are isolated only to a single patch. It's just your
f1ffa71e8f (repack: add `--filter=<filter-spec>` option, 2023-09-11),
and in particular the `write_filtered_pack()` function.
I started messing around with it myself and generated the following
fixup! which can be applied on top of your version of f1ffa71e8f. It's
mostly straightforward, but there is a gotcha that the loop over
non-kept packs has to change to:
for_each_string_list_item(item, &existing->non_kept_packs)
/* ... */
for_each_string_list_item(item, &existing->cruft_packs)
/* ... */
, instead of just the first loop over non_kept_packs, since cruft packs
are stored in a separate list.
In any event, here's the fixup! I generated on top of that patch:
--- 8< ---
Subject: [PATCH] fixup! repack: add `--filter=<filter-spec>` option
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
builtin/repack.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 120f4241c0..0d23323d05 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -834,15 +834,13 @@ static int finish_pack_objects_cmd(struct child_process *cmd,
static int write_filtered_pack(const struct pack_objects_args *args,
const char *destination,
const char *pack_prefix,
- struct string_list *keep_pack_list,
- struct string_list *names,
- struct string_list *existing_packs,
- struct string_list *existing_kept_packs)
+ struct existing_packs *existing,
+ struct string_list *names)
{
struct child_process cmd = CHILD_PROCESS_INIT;
struct string_list_item *item;
FILE *in;
- int ret, i;
+ int ret;
const char *caret;
const char *scratch;
int local = skip_prefix(destination, packdir, &scratch);
@@ -853,9 +851,8 @@ static int write_filtered_pack(const struct pack_objects_args *args,
if (!pack_kept_objects)
strvec_push(&cmd.args, "--honor-pack-keep");
- for (i = 0; i < keep_pack_list->nr; i++)
- strvec_pushf(&cmd.args, "--keep-pack=%s",
- keep_pack_list->items[i].string);
+ for_each_string_list_item(item, &existing->kept_packs)
+ strvec_pushf(&cmd.args, "--keep-pack=%s", item->string);
cmd.in = -1;
@@ -872,10 +869,12 @@ static int write_filtered_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->cruft_packs)
fprintf(in, "%s.pack\n", item->string);
caret = pack_kept_objects ? "" : "^";
- for_each_string_list_item(item, existing_kept_packs)
+ for_each_string_list_item(item, &existing->kept_packs)
fprintf(in, "%s%s.pack\n", caret, item->string);
fclose(in);
@@ -1261,10 +1260,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
ret = write_filtered_pack(&po_args,
packtmp,
find_pack_prefix(packdir, packtmp),
- &keep_pack_list,
- &names,
- &existing_nonkept_packs,
- &existing_kept_packs);
+ &existing,
+ &names);
if (ret)
goto cleanup;
}
--
2.42.0.137.g6fe1dff026
--- >8 ---
Thanks,
Taylor
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox