* Re: [PATCH 1/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-11 21:54 UTC (permalink / raw)
To: René Scharfe
Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
Calvin Wan, Carlo Marcelo Arenas Belón, Elijah Newren,
Jeff King, Linus Torvalds, Mathias Krause, Taylor Blau, git
In-Reply-To: <e214eb9c-7576-f8f5-ef1d-3828affd47d8@web.de>
Hi René,
Thanks for feedback. I am already sleeping but let me try to reply anyway,
even if I don't really understand you.
On 09/11, René Scharfe wrote:
>
> Am 11.09.23 um 14:12 schrieb Oleg Nesterov:
> > show_funcname_line() returns when "lno <= opt->last_shown" and this
> > is not right in that the ->last_shown line (which matched the pattern)
> > can also have the actual function name we need to report.
> >
> > Change this code to check "lno < opt->last_shown". While at it, move
> > this check up to avoid the unnecessary "find the previous bol" loop.
> >
> > Note that --lno can't underflow, lno==0 is not possible in this loop.
> >
> > Simple test-case:
> >
> > $ cat TEST.c
> > void func(void);
> >
> > void func1(xxx)
> > {
> > use1(xxx);
> > }
> >
> > void func2(xxx)
> > {
> > use2(xxx);
> > }
> >
> > $ git grep --untracked -pn xxx TEST.c
> >
> > before the patch:
> >
> > TEST.c=1=void func(void);
> > TEST.c:3:void func1(xxx)
> > TEST.c:5: use1(xxx);
> > TEST.c:8:void func2(xxx)
> > TEST.c:10: use2(xxx);
> >
> > after the patch:
> >
> > TEST.c=1=void func(void);
> > TEST.c:3:void func1(xxx)
> > TEST.c=3=void func1(xxx)
> > TEST.c:5: use1(xxx);
> > TEST.c:8:void func2(xxx)
> > TEST.c=8=void func2(xxx)
> > TEST.c:10: use2(xxx);
> >
> > which looks much better to me.
>
> Interesting idea to treat function lines as annotations of matches
> instead of as special context.
Sorry, I don't understand... Let me repeat I am not familiar with this
code and terminology. Could you spell please?
> Showing lines twice feels wasteful, but
> at least for -p it might be justifiable from that angle.
Just in case... say, "func1" is reported twice only when it is really
needed. From the "after the patch" output above:
TEST.c:3:void func1(xxx)
this is what we already have without this patch
TEST.c=3=void func1(xxx)
this is what we have with this patch because the next
TEST.c:5: use1(xxx);
line needs the proper funcname line, and without this patch it would be
"void func()" which has nothing to do with use1(xxx),
If I do, say,
./git grep --untracked -pn func1 TEST.c
then (with or without this patch) the output is
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
in this case there is no reason to report "=void func1(xxx)".
> Wouldn't you
> have to repeat function line 3 before the match in line 8, though?
Why?
> The reason for not repeating a matched function line was that it
> doesn't add much information under the assumption that it's easy to
> identify function lines visually.
But it is not. Lets look again at the "before the patch:" output above,
TEST.c=1=void func(void);
TEST.c:3:void func1(xxx)
TEST.c:5: use1(xxx);
TEST.c:8:void func2(xxx)
TEST.c:10: use2(xxx);
it looks as if every "xxx" match is inside the (unrelated) func().
OK, "visually" you can also notice the "void funcX(xxx)" declarations
and understand whats going on.
But a) I don't think this is always easy, and b) it is certainly not
easy when you use "git-grep -p" in scripts. Please see 0/1.
> The patch would need to update Documentation/git-grep.txt as well to
> reflect the changed output.
Hmm... From Documentation/git-grep.txt:
-p::
--show-function::
Show the preceding line that contains the function name of
the match, unless the matching line is a function name itself.
...
this is still true after this patch. How do you think I should update this
section?
Oleg.
^ permalink raw reply
* Re: [RFC] New configuration option "diff.statNameWidth"
From: Dragan Simic @ 2023-09-12 0:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo7i8s48f.fsf@gitster.g>
On 2023-09-12 01:02, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> On 2023-09-03 05:43, Dragan Simic wrote:
>>> On 2023-09-03 00:16, Junio C Hamano wrote:
>>>> Having said all the above, once we start seeing the actual patches
>>>> and its sales pitch (aka proposed commit log message), we do guide
>>>> and help polishing the patch into a better shape, so it will not
>>>> be like the contributor has to work in the dark without knowing
>>>> what level of bar their contribution has to pass.
>>> Thanks, everything sounds great and welcoming to the new
>>> contributors,
>>> who of course need to be willing to put in the required amount of
>>> skill and effort.
>>
>> I sent a patch to the git mailing list today, about five hours ago,
>> but it hasn't appeared on the list yet. Could something be wrong with
>> the mail server(s), as I also received no other messages from the list
>> in the last six hours or so?
>
> It is a high-volume mailing list server and occasional hiccup is
> part of regular life. Waiting a bit and then poking the postmater
> at vger.kernel.org may be needed from time to time, but I am seeing
> your message on the archive, so it seems "waiting" has worked fine?
Yes, remaining patient did the trick this time. A whole lot of messages
from the list arrived at once after the hiccup resolved itself.
^ permalink raw reply
* Re: [PATCH] completion: improve doc for complex aliases
From: Linus Arver @ 2023-09-12 1:04 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget, git; +Cc: Steffen Prohaska, Philippe Blain
In-Reply-To: <pull.1585.git.1694274592854.gitgitgadget@gmail.com>
Hi Philippe,
"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. The examples have that space but it's not clear if it's just for
> style or if it's mandatory.
>
> Explicitely mention it.
It would be even more helpful if you explain _why_ it is mandatory in
the commit message. Is there some Bash-specific behavior or something
else going on here?
If you are unable to explain why, then as an alternative you could
explain the error or buggy behavior (any error messages encountered, for
example) you observe on your system when you do not use the space (which
is corrected by applying the suggestion you are adding in this patch).
Thanks!
^ permalink raw reply
* Re: [bug] git clone command leaves orphaned ssh process
From: Aaron Schrab @ 2023-09-12 0:40 UTC (permalink / raw)
To: Max Amelchenko
Cc: Taylor Blau, Bagas Sanjaya, git, Hideaki Yoshifuji,
Junio C Hamano
In-Reply-To: <CAN47KsX5cpo5oD7PAwAQzjR4oocST6uSkJe2SzAYPxxqy7dGtg@mail.gmail.com>
At 13:11 +0300 11 Sep 2023, Max Amelchenko <maxamel2002@gmail.com> wrote:
>Maybe it's connected also to the underlying infrastructure? We are
>getting this in AWS lambda jobs and we're hitting a system limit of
>max processes because of it.
Running as a lambda, or in a container, could definitely be why you're
seeing a difference. Normally when a process is orphaned it gets adopted
by `init` (PID 1), and that will take care of cleaning up after orphaned
zombie processes.
But most of the time containers just run the configured process
directly, without an init process. That leaves nothing to clean orphan
processes.
Although for that to really be a problem, would require hitting that max
process limit inside a single container invocation. Of course since
containers usually aren't meant to be spawning a lot of processes, that
limit might be a lot lower than on a normal system.
I know that Docker provides a way to include an init process in the
started container (`docker run --init`), but I don't think that AWS
Lambda does.
^ permalink raw reply
* Re: [PATCH 01/32] doc hash-file-transition: A map file for mapping between sha1 and sha256
From: brian m. carlson @ 2023-09-12 0:14 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, Junio C Hamano
In-Reply-To: <20230908231049.2035003-1-ebiederm@xmission.com>
[-- Attachment #1: Type: text/plain, Size: 3718 bytes --]
On 2023-09-08 at 23:10:18, Eric W. Biederman wrote:
> The v3 pack index file as documented has a lot of complexity making it
> difficult to implement correctly. I worked with bryan's preliminary
> implementation and it took several passes to get the bugs out.
>
> The complexity also requires multiple table look-ups to find all of
> the information that is needed to translate from one kind of oid to
> another. Which can't be good for cache locality.
>
> Even worse coming up with a new index file version requires making
> changes that have the potentialy to break anything that uses the index
> of a pack file.
>
> Instead of continuing to deal with the chance of braking things
> besides the oid mapping functionality, the additional complexity in
> the file format, and worry if the performance would be reasonable I
> stripped down the problem to it's fundamental complexity and came up
> with a file format that is exactly about mapping one kind of oid to
> another, and only supports two kinds of oids.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> .../technical/hash-function-transition.txt | 40 +++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/Documentation/technical/hash-function-transition.txt b/Documentation/technical/hash-function-transition.txt
> index ed574810891c..4b937480848a 100644
> --- a/Documentation/technical/hash-function-transition.txt
> +++ b/Documentation/technical/hash-function-transition.txt
> @@ -209,6 +209,46 @@ format described in linkgit:gitformat-pack[5], just like
> today. The content that is compressed and stored uses SHA-256 content
> instead of SHA-1 content.
>
> +Per Pack Mapping Table
> +~~~~~~~~~~~~~~~~~~~~~~
> +A pack compat map file (.compat) files have the following format:
> +
> +HEADER:
> + 4-byte signature:
> + The signature is: {'C', 'M', 'A', 'P'}
> + 1-byte version number:
> + Git only writes or recognizes version 1.
> + 1-byte First Object Id Version
> + We infer the length of object IDs (OIDs) from this value:
> + 1 => SHA-1
> + 2 => SHA-256
One thing I forgot to mention here, is that we have 32-bit format IDs
for these in the structure, so we should use them here and below. These
are GIT_SHA1_FORMAT_ID and GIT_SHA256_FORMAT_ID.
Not that I would encourage distributing such software, but it makes it
much easier for people to experiment with additional hash algorithms (in
terms of performance, etc.) if we make the space a little sparser.
> + 1-byte Second Object Id Version
> + We infer the length of object IDs (OIDs) from this value:
> + 1 => SHA-1
> + 2 => SHA-256
In your new patch for the next part, you consider that there might be
multiple compatibility hash algorithms. I had anticipated only one at
a time in my series, but I'm not opposed to multiple if you want to
support that.
However, here you're making the assumption that there are only two. If
you want to support multiple values, we need to explicitly consider that
both here (where we need a count of object ID version and multiple
tables, one for each algorithm), and in the follow-up series.
I had not considered more than two algorithms because it substantially
complicates the code and requires us to develop n*(n-1) tables, but I'm
not the one volunteering to do most of the work here, so I'll defer to
your preference. (I do intend to send a patch or two, though.)
It's also possible we could be somewhat provident and define the on-disk
formats for multiple algorithms and then punt on the code until later if
you prefer that.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply
* Re: Commit dates on conflict markers
From: Junio C Hamano @ 2023-09-11 23:31 UTC (permalink / raw)
To: Roger Light; +Cc: git, Elijah Newren
In-Reply-To: <CAH7zdydYgSf+21GB70=gRhEcupv4e1ix==7LWCeQYgpD-1Rcmw@mail.gmail.com>
Roger Light <roger@atchoo.org> writes:
> When I carry out a merge with conflicts, it's not always clear when
> resolving the conflicts which is the correct part of code to use. I
> sometimes use git blame to guide me as to the age of the different
> chunks of code and hence what to choose.
>
> I was wondering if there might be a way to help include that sort of
> information directly into the conflict.
>
> If you had a single line conflict it would be straightforward to
> display by including the date the line was last modified alongside the
> conflict marker:
>
> <<<<<<< HEAD date:yesterday
> print("please")
> ======= date:10 years ago
> print("help")
> >>>>>>> main
>
> With a more realistic change with multiple lines and context from
> different commits, it's not immediately obvious to me that it's
> possible to do in a way that isn't completely horrible.
Our conflict marker lines do get human readable labels but the
format used by merge_3way() both in merge-ort and merge-recursive
backends is hardcoded to be <branchname> ':' <pathname> and it is
sufficient to let you tell which commit involved in the merge and
which path in that commit the contents came from.
A change that only shows the commit date without allowing end user
configuration will *not* be worth doing, but allowing them to use
placeholders like '%h %s' in "git log --format='%h %s'" (check
pretty.c for the catalog) would be a good exercise; it should not
take somebody with an ultra-deep knowledge of how the code works.
^ permalink raw reply
* Re: [PATCH 1/2] diff-merges: improve --diff-merges documentation
From: Junio C Hamano @ 2023-09-11 21:12 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <20230909125446.142715-2-sorganov@gmail.com>
Sergey Organov <sorganov@gmail.com> writes:
> ifdef::git-log[]
> ---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::
> +-m::
> + Show diffs for merge commits in the default format. This is
> + similar to '--diff-merges=on' (which see) except `-m` will
> + produce no output unless `-p` is given as well.
> ++
> +Note: This option not implying `-p` is legacy feature that is
> +preserved for the sake of backward compatibility.
It is more like that `-p` does not imply `-m` (which used to mean
"consider showing the comparison between parent(s) and the child,
even for merge commits"), even though newer options like `-c`,
`--cc` and others do imply `-m` (simply because they do not make
much sense if they are not allowed to work on merges) that may make
new people confused. If `-p` implied `-m` (or if `-m` implied
`-p`), it would also have been utterly confusing and useless for
human consumption.
In either case, unless the reason why `-p` does not imply `-m`
unlike others is explained, I do not think the note adds that much
value. I'd suggest dropping it.
> --no-diff-merges::
> + Synonym for '--diff-merges=off'.
> +
> +--diff-merges=<format>::
> Specify diff format to be used for merge commits. Default is
> - {diff-merges-default} unless `--first-parent` is in use, in which case
> - `first-parent` is the default.
> + {diff-merges-default} unless `--first-parent` is in use, in
> + which case `first-parent` is the default.
> +
> +The following formats are supported:
> ++
> +--
> +off, none::
> Disable output of diffs for merge commits. Useful to override
> implied value.
> +
> +on, m::
> + Make diff output for merge commits to be shown in the default
> + format. The default format could be changed using
> `log.diffMerges` configuration parameter, which default value
> is `separate`.
> +
> +first-parent, 1::
> + Show full diff with respect to first parent. This is the same
> + format as `--patch` produces for non-merge commits.
> +
> +separate::
> + Show full diff with respect to each of parents.
> + Separate log entry and diff is generated for each parent.
> +
> +remerge, r::
> + Remerge two-parent merge commits to create a temporary tree
> + object--potentially containing files with conflict markers
> + and such. A diff is then shown between that temporary tree
> + and the actual merge commit.
> +
> The output emitted when this option is used is subject to change, and
> so is its interaction with other options (unless explicitly
> documented).
> +
> +combined, c::
> + Show differences from each of the parents to the merge
> + result simultaneously instead of showing pairwise diff between
> + a parent and the result one at a time. Furthermore, it lists
> + only files which were modified from all parents.
> +
> +dense-combined, cc::
> + Further compress output produced by `--diff-merges=combined`
> + by omitting uninteresting hunks whose contents in the parents
> + have only two variants and the merge result picks one of them
> + without modification.
> +--
Looks reasonable, even though I didn't quite see much problem with
the original. If we were shuffling the sections like this patch, I
wonder if moving combined/dense-combined a bit higher (perhaps
before the "remerge") may make more sense, though (the ordering
would simply become "simpler to more involved").
^ permalink raw reply
* [PATCH 0/1] git-grep: improve the --show-function behaviour
From: Oleg Nesterov @ 2023-09-11 12:11 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Calvin Wan,
Carlo Marcelo Arenas Belón, Elijah Newren, Jeff King,
Linus Torvalds, Mathias Krause, René Scharfe, Taylor Blau,
git
Hello,
I have never looked into the git sources before, so I am spamming
a lot of people found in git-log grep.c
Yesterday I tried to teach my text editor to use "git grep -p" and
I noticed that it doesn't work as I'd expect. The changelog has a
simple test-case, but let me also provide "real life" example.
When I do
:git-grep -pw pid kernel/sys.c
in my editor before 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.
After 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_setpgid struct pid *pgrp;
kernel/sys.c 1080 sys_setpgid if (!pid)
kernel/sys.c 1081 sys_setpgid pid = task_pid_vnr(group_leader);
kernel/sys.c 1083 sys_setpgid pgid = pid;
kernel/sys.c 1094 sys_setpgid p = find_task_by_vpid(pid);
kernel/sys.c 1120 sys_setpgid if (pgid != pid) {
kernel/sys.c 1144 static int do_getpgid(pid_t pid)
kernel/sys.c 1147 do_getpgid struct pid *grp;
kernel/sys.c 1151 do_getpgid if (!pid)
kernel/sys.c 1155 do_getpgid p = find_task_by_vpid(pid);
kernel/sys.c 1172 SYSCALL_DEFINE1(getpgid, pid_t, pid)
kernel/sys.c 1174 sys_getpgid return do_getpgid(pid);
kernel/sys.c 1186 SYSCALL_DEFINE1(getsid, pid_t, pid)
kernel/sys.c 1189 sys_getsid struct pid *sid;
kernel/sys.c 1193 sys_getsid if (!pid)
kernel/sys.c 1197 sys_getsid p = find_task_by_vpid(pid);
kernel/sys.c 1214 static void set_special_pids(struct pid *pid)
kernel/sys.c 1218 set_special_pids if (task_session(curr) != pid)
kernel/sys.c 1219 set_special_pids change_pid(curr, PIDTYPE_SID, pid);
kernel/sys.c 1221 set_special_pids if (task_pgrp(curr) != pid)
kernel/sys.c 1222 set_special_pids 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 sys_prlimit64 tsk = pid ? find_task_by_vpid(pid) : current;
and everythig looks correct.
Oleg.
^ permalink raw reply
* [PATCH v6 9/9] gc: add `gc.repackFilterTo` config option
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
A previous commit implemented the `gc.repackFilter` config option
to specify a filter that should be used by `git gc` when
performing repacks.
Another previous commit has implemented
`git repack --filter-to=<dir>` to specify the location of the
packfile containing filtered out objects when using a filter.
Let's implement the `gc.repackFilterTo` config option to specify
that location in the config when `gc.repackFilter` is used.
Now when `git gc` will perform a repack with a <dir> configured
through this option and not empty, the repack process will be
passed a corresponding `--filter-to=<dir>` argument.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/config/gc.txt | 11 +++++++++++
builtin/gc.c | 4 ++++
t/t6500-gc.sh | 13 ++++++++++++-
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt
index 2153bde7ac..466466d6cc 100644
--- a/Documentation/config/gc.txt
+++ b/Documentation/config/gc.txt
@@ -150,6 +150,17 @@ gc.repackFilter::
objects into a separate packfile. See the
`--filter=<filter-spec>` option of linkgit:git-repack[1].
+gc.repackFilterTo::
+ When repacking and using a filter, see `gc.repackFilter`, the
+ specified location will be used to create the packfile
+ containing the filtered out objects. **WARNING:** The
+ specified location should be accessible, using for example the
+ Git alternates mechanism, otherwise the repo could be
+ considered corrupt by Git as it migh not be able to access the
+ objects in that packfile. See the `--filter-to=<dir>` option
+ of linkgit:git-repack[1] and the `objects/info/alternates`
+ section of linkgit:gitrepository-layout[5].
+
gc.rerereResolved::
Records of conflicted merge you resolved earlier are
kept for this many days when 'git rerere gc' is run.
diff --git a/builtin/gc.c b/builtin/gc.c
index 607c0ac23e..8aad103b45 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -62,6 +62,7 @@ static const char *gc_log_expire = "1.day.ago";
static const char *prune_expire = "2.weeks.ago";
static const char *prune_worktrees_expire = "3.months.ago";
static char *repack_filter;
+static char *repack_filter_to;
static unsigned long big_pack_threshold;
static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
@@ -172,6 +173,7 @@ static void gc_config(void)
git_config_get_ulong("pack.deltacachesize", &max_delta_cache_size);
git_config_get_string("gc.repackfilter", &repack_filter);
+ git_config_get_string("gc.repackfilterto", &repack_filter_to);
git_config(git_default_config, NULL);
}
@@ -361,6 +363,8 @@ static void add_repack_all_option(struct string_list *keep_pack)
if (repack_filter && *repack_filter)
strvec_pushf(&repack, "--filter=%s", repack_filter);
+ if (repack_filter_to && *repack_filter_to)
+ strvec_pushf(&repack, "--filter-to=%s", repack_filter_to);
}
static void add_repack_incremental_option(void)
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 232e403b66..e412cf8daf 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -203,7 +203,6 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e
'
test_expect_success 'gc.repackFilter launches repack with a filter' '
- test_when_finished "rm -rf bare.git" &&
git clone --no-local --bare . bare.git &&
git -C bare.git -c gc.cruftPacks=false gc &&
@@ -215,6 +214,18 @@ test_expect_success 'gc.repackFilter launches repack with a filter' '
grep -E "^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*" trace.out
'
+test_expect_success 'gc.repackFilterTo store filtered out objects' '
+ test_when_finished "rm -rf bare.git filtered.git" &&
+
+ git init --bare filtered.git &&
+ git -C bare.git -c gc.repackFilter=blob:none \
+ -c gc.repackFilterTo=../filtered.git/objects/pack/pack \
+ -c repack.writeBitmaps=false -c gc.cruftPacks=false gc &&
+
+ test_stdout_line_count = 1 ls bare.git/objects/pack/*.pack &&
+ test_stdout_line_count = 1 ls filtered.git/objects/pack/*.pack
+'
+
prepare_cruft_history () {
test_commit base &&
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* Re: [PATCH 0/2] replacing ci/config/allow-ref with a repo variable
From: Jeff King @ 2023-09-11 9:36 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Johannes Schindelin
In-Reply-To: <fcdc682b-cf6c-4db9-9970-be136f48de58@gmail.com>
On Thu, Sep 07, 2023 at 11:04:33AM +0100, Phillip Wood wrote:
> I scanned the github documentation the other day and wondered if it would be
> possible to use with fromJson with a json array to do a prefx match on each
> element. It all sounds like it is getting a bit complicated though.
I poked around and I don't think this is possible. You can use
contains() to do a full match of items in a json array, but I don't see
any other way to iterate. What we'd really want is a "map" function, to
do something like:
map { startsWith(github.ref_name, $1) } fromJSON(vars.CI_CONFIG.allow_prefix)
But no such "map" exists (and I guess we'd need to collapse the result
down to "is any item true", similar to perl's "grep" function).
Do you need multiple prefixes, or would one each of allow/reject be
sufficient? I tried this and it works:
jobs:
ci-config:
name: config
if: |
(fromJSON(vars.CI_CONFIG).allow == '' ||
contains(fromJSON(vars.CI_CONFIG).allow, github.ref_name)) &&
(fromJSON(vars.CI_CONFIG).reject == '' ||
!contains(fromJSON(vars.CI_CONFIG).reject, github.ref_name)) &&
(fromJSON(vars.CI_CONFIG).allow-prefix == '' ||
startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).allow-prefix)) &&
(fromJSON(vars.CI_CONFIG).reject-prefix == '' ||
!startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).reject-prefix))
And then various values of CI_CONFIG seem to work:
- allow explicit branch names ("one" and "two" are run, everything
else is skipped)
{ "allow": ["one", "two"] }
- reject explicit names (everything except "three" is skipped)
{ "reject": ["three"] }
- allow by prefix ("ok/* is run, everything else is skipped)
{ "allow-prefix": "ok/" }
- reject by prefix (everything except "nope/*" is run)
{ "reject-prefix": "nope/" }
- every key must approve to run, but missing keys default to running.
So a reject overrides allow ("ok/one" and "ok/two" run, but
"ok/three" does not)
{
"allow-prefix": "ok/",
"reject", "ok/three"
}
I suppose one hacky way to support multiple prefixes is to just unroll
the loop ourselves (since missing keys are ignored). I didn't test it,
but something like:
(fromJSON(vars.CI_CONFIG).allow-prefix[0] == '' ||
startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).allow-prefix[0])) &&
(fromJSON(vars.CI_CONFIG).allow-prefix[1] == '' ||
startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).allow-prefix[1])) &&
(fromJSON(vars.CI_CONFIG).allow-prefix[2] == '' ||
startsWith(github.ref_name, fromJSON(vars.CI_CONFIG).allow-prefix[2])) &&
...etc...
Would allow:
{
"allow-prefix": [
"ok/",
"also-ok/"
]
}
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, or would it be
sufficient to just reject "refs/tags/" completely (though that implies
using the fully qualified ref and not the short name; it would also be
easy to add a boolean "allow tags" config option).
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] parse-options: add int value pointer to struct option
From: Junio C Hamano @ 2023-09-11 19:19 UTC (permalink / raw)
To: Taylor Blau; +Cc: René Scharfe, Git List, Jeff King
In-Reply-To: <ZP4NrVeqMtFTLEuf@nand.local>
Taylor Blau <me@ttaylorr.com> writes:
> callback, something like:
>
> struct option {
> /* ... */
> union {
> void *value;
> int *value_int;
> /* etc ... */
> } u;
> enum option_type t;
> };
>
> where option_type has some value corresponding to "void *", another for
> "int *", and so on.
Yup, that does cross my mind, even though I would have used
union {
void *void_ptr;
int *int_ptr;
} value;
or something without a rather meaningless 'u'.
> Alternatively, perhaps you are thinking that we'd use both the value
> pointer and the value_int pointer to point at potentially different
> values in the same callback. I don't have strong feelings about it, but
> I'd just as soon encourage us to shy away from that approach, since
> assigning a single callback parameter to each function seems more
> organized.
We have seen (with Peff's "-Wunused" work) that there are small
number of cases that it would be handy for a callback to be told the
locations of multiple external variables, but I do not think it
would be a good solution to that problem to have "void *value" and
"int value_int" next to each other and allow them to coexist, as it
would work only when these multiple variables happen to be of the
right types.
^ permalink raw reply
* Re: [PATCH v2 6/6] trailer: use offsets for trailer_start/trailer_end
From: Junio C Hamano @ 2023-09-11 19:01 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <0463066ebe0889b72b6a1f6c344f2de127458391.1694240177.git.gitgitgadget@gmail.com>
"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, 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.
So, I dunno.
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Junio C Hamano @ 2023-09-11 19:12 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King
In-Reply-To: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
René Scharfe <l.s.r@web.de> writes:
> Some uses of OPT_CMDMODE provide a pointer to an enum. It is
> dereferenced as an int pointer in parse-options.c::get_value(). These
> two types are incompatible, though -- the storage size of an enum can
> vary between platforms. C23 would allow us to specify the underlying
> type of the different enums, making them compatible, but with C99 the
> easiest safe option is to actually use int as the value type.
>
> Convert the offending OPT_CMDMODE users and use the typed value_int
> point in the macro's definition to enforce that type for future ones.
Interesting. I wondered if this means that applying [1/2] alone
will immediately break these places that [2/2] fixes, but the answer
is no, as the previous step did not make these places use the typed
pointer. But it also means that with this step alone to use "int",
instead of various "enum" types that can have representations that
are different from "int", would already "fix" the current code
while still casing back and forth from (void *)?
In any case, the two-patch series looks good, and it does not break
bisectability, either.
Thanks.
^ permalink raw reply
* [PATCH v6 3/9] repack: refactor finishing pack-objects command
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
Create a new finish_pack_objects_cmd() to refactor duplicated code
that handles reading the packfile names from the output of a
`git pack-objects` command and putting it into a string_list, as well as
calling finish_command().
While at it, beautify a code comment a bit in the new function.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org
---
builtin/repack.c | 70 +++++++++++++++++++++++-------------------------
1 file changed, 33 insertions(+), 37 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 6943c5ba11..4f53b24958 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -695,6 +695,36 @@ static void remove_redundant_bitmaps(struct string_list *include,
strbuf_release(&path);
}
+static int finish_pack_objects_cmd(struct child_process *cmd,
+ struct string_list *names,
+ int local)
+{
+ FILE *out;
+ struct strbuf line = STRBUF_INIT;
+
+ out = xfdopen(cmd->out, "r");
+ while (strbuf_getline_lf(&line, out) != EOF) {
+ struct string_list_item *item;
+
+ if (line.len != the_hash_algo->hexsz)
+ die(_("repack: Expecting full hex object ID lines only "
+ "from pack-objects."));
+ /*
+ * Avoid putting packs written outside of the repository in the
+ * list of names.
+ */
+ if (local) {
+ item = string_list_append(names, line.buf);
+ item->util = populate_pack_exts(line.buf);
+ }
+ }
+ fclose(out);
+
+ strbuf_release(&line);
+
+ return finish_command(cmd);
+}
+
static int write_cruft_pack(const struct pack_objects_args *args,
const char *destination,
const char *pack_prefix,
@@ -704,9 +734,8 @@ static int write_cruft_pack(const struct pack_objects_args *args,
struct string_list *existing_kept_packs)
{
struct child_process cmd = CHILD_PROCESS_INIT;
- struct strbuf line = STRBUF_INIT;
struct string_list_item *item;
- FILE *in, *out;
+ FILE *in;
int ret;
const char *scratch;
int local = skip_prefix(destination, packdir, &scratch);
@@ -749,27 +778,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,
fprintf(in, "%s.pack\n", item->string);
fclose(in);
- out = xfdopen(cmd.out, "r");
- while (strbuf_getline_lf(&line, out) != EOF) {
- struct string_list_item *item;
-
- if (line.len != the_hash_algo->hexsz)
- die(_("repack: Expecting full hex object ID lines only "
- "from pack-objects."));
- /*
- * avoid putting packs written outside of the repository in the
- * list of names
- */
- if (local) {
- item = string_list_append(names, line.buf);
- item->util = populate_pack_exts(line.buf);
- }
- }
- fclose(out);
-
- strbuf_release(&line);
-
- return finish_command(&cmd);
+ return finish_pack_objects_cmd(&cmd, names, local);
}
int cmd_repack(int argc, const char **argv, const char *prefix)
@@ -780,10 +789,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct string_list existing_nonkept_packs = STRING_LIST_INIT_DUP;
struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
struct pack_geometry geometry = { 0 };
- struct strbuf line = STRBUF_INIT;
struct tempfile *refs_snapshot = NULL;
int i, ext, ret;
- FILE *out;
int show_progress;
/* variables to be filled by option parsing */
@@ -1013,18 +1020,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
fclose(in);
}
- out = xfdopen(cmd.out, "r");
- while (strbuf_getline_lf(&line, out) != EOF) {
- struct string_list_item *item;
-
- if (line.len != the_hash_algo->hexsz)
- die(_("repack: Expecting full hex object ID lines only from pack-objects."));
- item = string_list_append(&names, line.buf);
- item->util = populate_pack_exts(item->string);
- }
- strbuf_release(&line);
- fclose(out);
- ret = finish_command(&cmd);
+ ret = finish_pack_objects_cmd(&cmd, &names, 1);
if (ret)
goto cleanup;
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* [PATCH v6 7/9] gc: add `gc.repackFilter` config option
From: Christian Couder @ 2023-09-11 15:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, John Cai, Jonathan Tan, Jonathan Nieder,
Taylor Blau, Derrick Stolee, Patrick Steinhardt, Christian Couder,
Christian Couder
In-Reply-To: <20230911150618.129737-1-christian.couder@gmail.com>
A previous commit has implemented `git repack --filter=<filter-spec>` to
allow users to filter out some objects from the main pack and move them
into a new different pack.
Users might want to perform such a cleanup regularly at the same time as
they perform other repacks and cleanups, so as part of `git gc`.
Let's allow them to configure a <filter-spec> for that purpose using a
new gc.repackFilter config option.
Now when `git gc` will perform a repack with a <filter-spec> configured
through this option and not empty, the repack process will be passed a
corresponding `--filter=<filter-spec>` argument.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Documentation/config/gc.txt | 5 +++++
builtin/gc.c | 6 ++++++
t/t6500-gc.sh | 13 +++++++++++++
3 files changed, 24 insertions(+)
diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt
index ca47eb2008..2153bde7ac 100644
--- a/Documentation/config/gc.txt
+++ b/Documentation/config/gc.txt
@@ -145,6 +145,11 @@ Multiple hooks are supported, but all must exit successfully, else the
operation (either generating a cruft pack or unpacking unreachable
objects) will be halted.
+gc.repackFilter::
+ When repacking, use the specified filter to move certain
+ objects into a separate packfile. See the
+ `--filter=<filter-spec>` option of linkgit:git-repack[1].
+
gc.rerereResolved::
Records of conflicted merge you resolved earlier are
kept for this many days when 'git rerere gc' is run.
diff --git a/builtin/gc.c b/builtin/gc.c
index 369bd43fb2..607c0ac23e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -61,6 +61,7 @@ static timestamp_t gc_log_expire_time;
static const char *gc_log_expire = "1.day.ago";
static const char *prune_expire = "2.weeks.ago";
static const char *prune_worktrees_expire = "3.months.ago";
+static char *repack_filter;
static unsigned long big_pack_threshold;
static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
@@ -170,6 +171,8 @@ static void gc_config(void)
git_config_get_ulong("gc.bigpackthreshold", &big_pack_threshold);
git_config_get_ulong("pack.deltacachesize", &max_delta_cache_size);
+ git_config_get_string("gc.repackfilter", &repack_filter);
+
git_config(git_default_config, NULL);
}
@@ -355,6 +358,9 @@ static void add_repack_all_option(struct string_list *keep_pack)
if (keep_pack)
for_each_string_list(keep_pack, keep_one_pack, NULL);
+
+ if (repack_filter && *repack_filter)
+ strvec_pushf(&repack, "--filter=%s", repack_filter);
}
static void add_repack_incremental_option(void)
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 69509d0c11..232e403b66 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -202,6 +202,19 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e
grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out
'
+test_expect_success 'gc.repackFilter launches repack with a filter' '
+ test_when_finished "rm -rf bare.git" &&
+ git clone --no-local --bare . bare.git &&
+
+ git -C bare.git -c gc.cruftPacks=false gc &&
+ test_stdout_line_count = 1 ls bare.git/objects/pack/*.pack &&
+
+ GIT_TRACE=$(pwd)/trace.out git -C bare.git -c gc.repackFilter=blob:none \
+ -c repack.writeBitmaps=false -c gc.cruftPacks=false gc &&
+ test_stdout_line_count = 2 ls bare.git/objects/pack/*.pack &&
+ grep -E "^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*" trace.out
+'
+
prepare_cruft_history () {
test_commit base &&
--
2.42.0.167.gd6ff314189
^ permalink raw reply related
* Re: [PATCH] rebase -i: ignore signals when forking subprocesses
From: Oswald Buddenhagen @ 2023-09-11 10:32 UTC (permalink / raw)
To: phillip.wood
Cc: Jeff King, Phillip Wood via GitGitGadget, git,
Johannes Schindelin, Junio C Hamano
In-Reply-To: <a37a0f11-0723-4964-a187-54d960615d79@gmail.com>
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:
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.
a few notes on that:
- install without SA_RESTART, so syscalls can actually return with EINTR
and give us the opportunity to check the flag.
- an alternative to setting flags is setjmp()/longjmp(), but you really
don't want to go there.
- install with SA_RESETHAND, so the second ctrl-c will kill git
regardless, providing an escape hatch.
>In practice if the child is catching SIGINT or SIGQUIT we cannot rely
>on it re-raising the signal so that wont work.
>
yes, but that's a minor issue, i think.
by far most hooks and other things that might be invoked within
sequencer context don't mess with signals in the first place.
the things that do should be presumed to do the right thing, which means
at least a non-zero exit status in case of a premature termination,
which will yield pretty much the same effect on our side anyway.
so the only actually problematic situation would be us completely
ignoring the exit code (like the git-gc call, but that's clearly a bug
in git, and we control both sides, so it's easily fixable).
regards
^ permalink raw reply
* Re: [PATCH v2 1/6] trailer: separate public from internal portion of trailer_iterator
From: Junio C Hamano @ 2023-09-11 17:10 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <4f116d2550f6cf218477560a9e25dbe4c384a2a6.1694240177.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> The fields here are not meant to be used by downstream callers, so put
> them behind an anonymous struct named as "internal" to warn against
> their use. This follows the pattern in 576de3d956 (unpack_trees: start
> splitting internal fields from public API, 2023-02-27).
OK. The patch shows that there exist no code external to this file
that touch these members that are marked "private", so it has some
auditing value from that point of view, which is nice.
But that is only about today's code and does not protect us from
future breakage. In other words, "git grep internal\\." would not
be an effective way to find misuses of these members from the
sidelines. But that is OK, as "git grep -E '([.]|->)info'" would
not be an effective way in today's code, either, and the patch is
not making things worse.
Queued. Thanks.
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> trailer.c | 10 +++++-----
> trailer.h | 6 ++++--
> 2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index f408f9b058d..de4bdece847 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1220,14 +1220,14 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
> strbuf_init(&iter->key, 0);
> strbuf_init(&iter->val, 0);
> opts.no_divider = 1;
> - trailer_info_get(&iter->info, msg, &opts);
> - iter->cur = 0;
> + trailer_info_get(&iter->internal.info, msg, &opts);
> + iter->internal.cur = 0;
> }
>
> int trailer_iterator_advance(struct trailer_iterator *iter)
> {
> - while (iter->cur < iter->info.trailer_nr) {
> - char *trailer = iter->info.trailers[iter->cur++];
> + while (iter->internal.cur < iter->internal.info.trailer_nr) {
> + char *trailer = iter->internal.info.trailers[iter->internal.cur++];
> int separator_pos = find_separator(trailer, separators);
>
> if (separator_pos < 1)
> @@ -1245,7 +1245,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
>
> void trailer_iterator_release(struct trailer_iterator *iter)
> {
> - trailer_info_release(&iter->info);
> + trailer_info_release(&iter->internal.info);
> strbuf_release(&iter->val);
> strbuf_release(&iter->key);
> }
> diff --git a/trailer.h b/trailer.h
> index 795d2fccfd9..ab2cd017567 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -119,8 +119,10 @@ struct trailer_iterator {
> struct strbuf val;
>
> /* private */
> - struct trailer_info info;
> - size_t cur;
> + struct {
> + struct trailer_info info;
> + size_t cur;
> + } internal;
> };
>
> /*
^ permalink raw reply
* [PATCH v3] diff-lib: Fix check_removed when fsmonitor is on
From: Josip Sokcevic @ 2023-09-11 17:09 UTC (permalink / raw)
To: gitster, jonathantanmy; +Cc: git, git, Josip Sokcevic
In-Reply-To: <20230906060241.944886-2-sokcevic@google.com>
git-diff-index may return incorrect deleted entries when fsmonitor is used in a
repository with git submodules. This can be observed on Mac machines, but it
can affect all other supported platforms too.
If fsmonitor is used, `stat *st` is not initialized if cache_entry has
CE_FSMONITOR_VALID set. But, there are three call sites that rely on stat
afterwards, which can result in incorrect results.
This change partially reverts commit 4f3d6d0.
Signed-off-by: Josip Sokcevic <sokcevic@google.com>
---
diff-lib.c | 12 ++++++------
t/t7527-builtin-fsmonitor.sh | 5 +++++
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/diff-lib.c b/diff-lib.c
index d8aa777a73..5848e4f9ca 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -36,14 +36,14 @@
* exists for ce that is a submodule -- it is a submodule that is not
* checked out). Return negative for an error.
*/
-static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
+static int check_removed(const struct cache_entry *ce, struct stat *st)
{
- assert(is_fsmonitor_refreshed(istate));
- if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
+ if (lstat(ce->name, st) < 0) {
if (!is_missing_file_error(errno))
return -1;
return 1;
}
+
if (has_symlink_leading_path(ce->name, ce_namelen(ce)))
return 1;
if (S_ISDIR(st->st_mode)) {
@@ -149,7 +149,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
memset(&(dpath->parent[0]), 0,
sizeof(struct combine_diff_parent)*5);
- changed = check_removed(istate, ce, &st);
+ changed = check_removed(ce, &st);
if (!changed)
wt_mode = ce_mode_from_stat(ce, st.st_mode);
else {
@@ -229,7 +229,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
} else {
struct stat st;
- changed = check_removed(istate, ce, &st);
+ changed = check_removed(ce, &st);
if (changed) {
if (changed < 0) {
perror(ce->name);
@@ -303,7 +303,7 @@ static int get_stat_data(const struct index_state *istate,
if (!cached && !ce_uptodate(ce)) {
int changed;
struct stat st;
- changed = check_removed(istate, ce, &st);
+ changed = check_removed(ce, &st);
if (changed < 0)
return -1;
else if (changed) {
diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
index 0c241d6c14..78503158fd 100755
--- a/t/t7527-builtin-fsmonitor.sh
+++ b/t/t7527-builtin-fsmonitor.sh
@@ -809,6 +809,11 @@ my_match_and_clean () {
status --porcelain=v2 >actual.without &&
test_cmp actual.with actual.without &&
+ git -C super --no-optional-locks diff-index --name-status HEAD >actual.with &&
+ git -C super --no-optional-locks -c core.fsmonitor=false \
+ diff-index --name-status HEAD >actual.without &&
+ test_cmp actual.with actual.without &&
+
git -C super/dir_1/dir_2/sub reset --hard &&
git -C super/dir_1/dir_2/sub clean -d -f
}
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH] t3404-rebase-interactive.sh: fix name of a rewording test
From: Oswald Buddenhagen @ 2023-09-11 12:21 UTC (permalink / raw)
To: git; +Cc: Phillip Wood
The given test name made no sense to me at all; it seems to be a
concatenation of two unrelated things. This was introduced by
commit 0c164ae7a ("rebase -i: add another reword test", 20-08-20).
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
actually, i don't understand what the test even does. shouldn't it, to
match the description, actually dirty the tree and verify that the
operation fails?
Cc: Phillip Wood <phillip.wood@dunelm.org.uk>
---
t/t3404-rebase-interactive.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a56aafbe..31ee5bc1f6 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -758,7 +758,7 @@ test_expect_success 'reword' '
git show HEAD~2 | grep "C changed"
'
-test_expect_success 'no uncommited changes when rewording the todo list is reloaded' '
+test_expect_success 'no uncommitted changes when rewording' '
git checkout E &&
test_when_finished "git checkout @{-1}" &&
(
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related
* Re: [PATCH v2 5/6] trailer: rename *_DEFAULT enums to *_UNSPECIFIED
From: Junio C Hamano @ 2023-09-11 18:54 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <52958c3557c34992df59e9c10f098f457526702c.1694240177.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Do not use *_DEFAULT as a suffix to the enums, because the word
> "default" is overloaded. The following are two examples of the ambiguity
> of the word "default":
In this case these are left unspecified to use the default; while it
is not wrong per-se to say *_DEFAULT, using *_UNSPECIFIED makes it
more obvious.
> 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.
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)?
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-09-11 20:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jeff King
In-Reply-To: <xmqqedj4v808.fsf@gitster.g>
Am 11.09.23 um 21:12 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Some uses of OPT_CMDMODE provide a pointer to an enum. It is
>> dereferenced as an int pointer in parse-options.c::get_value(). These
>> two types are incompatible, though -- the storage size of an enum can
>> vary between platforms. C23 would allow us to specify the underlying
>> type of the different enums, making them compatible, but with C99 the
>> easiest safe option is to actually use int as the value type.
>>
>> Convert the offending OPT_CMDMODE users and use the typed value_int
>> point in the macro's definition to enforce that type for future ones.
>
> Interesting. I wondered if this means that applying [1/2] alone
> will immediately break these places that [2/2] fixes, but the answer
> is no, as the previous step did not make these places use the typed
> pointer. But it also means that with this step alone to use "int",
> instead of various "enum" types that can have representations that
> are different from "int", would already "fix" the current code
> while still casing back and forth from (void *)?
Yes and yes. And the change to use value_int on its own makes the type
mismatch visible via compiler warnings. It guards against future
violations.
René
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Phillip Wood @ 2023-09-11 9:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phillip Wood via GitGitGadget, git, Eric Wong, Phillip Wood
In-Reply-To: <xmqq8r9gtviv.fsf@gitster.g>
On 08/09/2023 18:38, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> Oh I should have looked more carefully at the existing uses. It looks
>> like it is only my sequencer patch that does
>>
>> sigchain_push(SIGINT, SIG_IGN);
>> sigchain_push(SIGQUIT, SIG_IGN);
>> res = run_command(...);
>
> Hmph, does it mean this patch would become unnecessary, once you fix
> the above sequence to follow the pattern "to spawn and then ignore"?
Yes, sorry for the confusion. There are a couple of things that I think
we should address though. Firstly we should change the comment in
run-command which says execve() resets ignored signals to SIG_DFL to say
something like
Preserve the set of ignored signals so that running git via a
wrapper like nohup works as the user expects
The other thing is that we have some instances where we ignore SIGPIPE
before calling start_command() which means we're ignoring it in the
child process as well. For example in gpg-interface.c we have
sigchain_push(SIGPIPE, SIG_IGN);
ret = pipe_command(&gpg, sigc->payload, sigc->payload_len, &gpg_stdout, 0,
&gpg_stderr, 0);
sigchain_pop(SIGPIPE);
To fix that one we'd need to change pipe_command() to ignore SIGPIPE
after calling start_command() or add a flag to struct child_process to
do to that.
Another example is in upload-pack.c
/*
* If the next rev-list --stdin encounters an unknown commit,
* it terminates, which will cause SIGPIPE in the write loop
* below.
*/
sigchain_push(SIGPIPE, SIG_IGN);
if (start_command(cmd))
goto error;
rev-list does not check for errors when writing to stdout unless
GIT_FLUSH is set in the environment so if parent process exits early
rev-list will keep going until it thinks it has printed everything.
I think adding a flag to struct child_process to ignore SIGPIPE in the
parent is probably the best way to avoid problems like this.
Best Wishes
Phillip
^ permalink raw reply
* Commit dates on conflict markers
From: Roger Light @ 2023-09-11 21:02 UTC (permalink / raw)
To: git
Hello,
When I carry out a merge with conflicts, it's not always clear when
resolving the conflicts which is the correct part of code to use. I
sometimes use git blame to guide me as to the age of the different
chunks of code and hence what to choose.
I was wondering if there might be a way to help include that sort of
information directly into the conflict.
If you had a single line conflict it would be straightforward to
display by including the date the line was last modified alongside the
conflict marker:
<<<<<<< HEAD date:yesterday
print("please")
======= date:10 years ago
print("help")
>>>>>>> main
With a more realistic change with multiple lines and context from
different commits, it's not immediately obvious to me that it's
possible to do in a way that isn't completely horrible.
I am in no way a git expert though, so I thought I'd ask and see what
you thought.
Regards,
Roger
^ permalink raw reply
* Re: [PATCH 1/4] cmake: also build unit tests
From: Phillip Wood @ 2023-09-11 13:23 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git; +Cc: Johannes Schindelin
In-Reply-To: <2cc1c03d85153356edd55fc0747a957db3c94ab0.1693462532.git.gitgitgadget@gmail.com>
Hi Johannes
On 31/08/2023 07:15, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> A new, better way to run unit tests was just added to Git. This adds
> support for building those unit tests via CMake.
This patch builds the unit tests but does not add them to the list of
tests run by CTest - how are the tests typically run on the CMake build?
Best Wishes
Phillip
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> contrib/buildsystems/CMakeLists.txt | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 2f6e0197ffa..45016213358 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -965,6 +965,24 @@ target_link_libraries(test-fake-ssh common-main)
> parse_makefile_for_sources(test-reftable_SOURCES "REFTABLE_TEST_OBJS")
> list(TRANSFORM test-reftable_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
>
> +#unit-tests
> +add_library(unit-test-lib OBJECT ${CMAKE_SOURCE_DIR}/t/unit-tests/test-lib.c)
> +
> +parse_makefile_for_scripts(unit_test_PROGRAMS "UNIT_TEST_PROGRAMS" "")
> +foreach(unit_test ${unit_test_PROGRAMS})
> + add_executable("${unit_test}" "${CMAKE_SOURCE_DIR}/t/unit-tests/${unit_test}.c")
> + target_link_libraries("${unit_test}" unit-test-lib common-main)
> + set_target_properties("${unit_test}"
> + PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/t/unit-tests)
> + if(MSVC)
> + set_target_properties("${unit_test}"
> + PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/t/unit-tests)
> + set_target_properties("${unit_test}"
> + PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/t/unit-tests)
> + endif()
> + list(APPEND PROGRAMS_BUILT "${unit_test}")
> +endforeach()
> +
> #test-tool
> parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
>
^ permalink raw reply
* Re: [PATCH v2 4/6] trailer: teach find_patch_start about --no-divider
From: Junio C Hamano @ 2023-09-11 17:25 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Glen Choo, Christian Couder, Phillip Wood, Linus Arver
In-Reply-To: <f5f507c4c6c4514af7dca35e307ca68e72435afb.1694240177.git.gitgitgadget@gmail.com>
"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.
The real purpose of this function is to find the end of the log
message, isn't it? 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.
^ 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