* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Elijah Newren @ 2023-11-14 2:50 UTC (permalink / raw)
To: Taylor Blau
Cc: Jeff King, git, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <ZVKkgpiFaOwwDcdw@nand.local>
On Mon, Nov 13, 2023 at 2:34 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Mon, Nov 13, 2023 at 05:02:54PM -0500, Jeff King wrote:
> > On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:
> >
> > > This is unsafe; the object may need to be read later within the same
> > > merge. [...]
> > >
> > > I think (untested) that you could fix this by creating two packs
> > > instead of just one. In particular, add a call to
> > > flush_odb_transcation() after the "redo_after_renames" block in
> > > merge_ort_nonrecursive_internal().
> >
> > It might not be too hard to just let in-process callers access the
> > objects we've written. A quick and dirty patch is below, which works
> > with the test you suggested (the test still fails because it finds a
> > conflict, but it gets past the "woah, I can't find that sha1" part).
>
> That's a very slick idea, and I think that this series has some legs to
> stand on now as a result.
>
> There are a few of interesting conclusions we can draw here:
>
> 1. This solves the recursive merge problem that Elijah mentioned
> earlier where we could generate up to 2^N packs (where N is the
> maximum depth of the recursive merge).
>
> 2. This also solves the case where the merge-ort code needs to read an
> object that it wrote earlier on in the same process without having
> to flush out intermediate packs. So in the best case we need only a
> single pack (instead of two).
>
> 3. This also solves the 'replay' issue, *and* allows us to avoid the
> tmp-objdir thing there entirely, since we can simulate object reads
> in the bulk-checkin pack.
>
> So I think that this is a direction worth pursuing.
Agreed; this looks great. It's basically bringing fast-import-like
functionality -- writing objects to a single new packfile while making
previous objects accessible to subsequent ones -- to additional
callers.
> In terms of making those lookups faster, I think that what you'd want is
> an oidmap. The overhead is slightly unfortunate, but I think that any
> other solution which requires keeping the written array in sorted order
> would in practice be more expensive as you have to constantly reallocate
> and copy portions of the array around to maintain its invariant.
When comparing the overhead of an oidmap to a bunch of inodes,
however, it seems relatively cheap. :-)
> > I don't know if that is sufficient, though. Would other spawned
> > processes (hooks, external merge drivers, and so on) need to be able to
> > see these objects, too?
>
> Interesting point. In theory those processes could ask about newly
> created objects, and if they were spawned before the bulk-checkin pack
> was flushed, those lookups would fail.
One of the big design differences that I was pushing really hard with
git-replay was performance and things that came with it -- no
worktree, no per-commit hooks (which are nearly ruled out by no
worktree, but it's still worth calling out separately), etc. A
post-operation hook could be fine, but would also not get to assume a
worktree.
merge-tree is the same as far as hooks; I'd rather just not have them,
but if we did, they'd be a post-operation hook.
In both cases, that makes hooks not much of a sticking point.
External merge drivers, however...
> But that requires that, e.g. for hooks, that we know a-priori the object
> ID of some newly-written objects. If you wanted to make those lookups
> succeed, I think there are a couple of options:
>
> - teach child-processes about the bulk-checkin pack, and let them
> perform the same fake lookup
>
> - flush (but do not close) the bulk-checkin transaction
>
> In any event, I think that this is a sufficiently rare and niche case
> that we'd be OK to declare that you should not expect the above
> scenarios to work when using `--write-pack`. If someone does ask for
> that feature in the future, we could implement it relatively painlessly
> using one of the above options.
Seems reasonable to me.
^ permalink raw reply
* Re: [PATCH 1/1] attr: add native file mode values support
From: Joanna Wang @ 2023-11-14 2:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Joanna Wang, tboegi
>> Even if we assume that this code is currently meant to work only
>> with GIT_ATTR_CHECKIN, I do not think this is what you want. When
>> asked to perform "git add . ':(exclude,mode=160000)'", you not only
>> want to exclude the submodules that are already known to this
>> superproject, but also a repository that _can_ become a submodule of
>> this superproject when added, no?
Sorry, I was totally ignorant of two key concepts that you mention here.
I somehow missed the concept of git_attr_direction altogether and I also
did not know submodules could be added with git-add (I was only
familiar with how
they are currently added in chromium via git update-index).
This makes sense now. I'll fix in the next patch.
>> On the other hand, the GIT_ATTR_CHECKOUT direction is hopefully much
>> simpler. You'd see what the path in the index is, among a gitlink,
>> a regular non-executable file, an executable file, or a symlink.
I noticed for both the GIT_ATTR_CHECKOUT and GIT_ATTR_CHECKIN directions,
in read_attr(), the indexed .gitattributes file is checked with the
actual file as fallback or vice versa.
I would think that we'd only want to use attributes from one state
(e.g. what's actually in the file)
or the other (e.g. what's indexed).
So I guess I'm still not sure what the "direction" concept is.
For GIT_ATTR_CHECKOUT, would we want to fallback to lstat?
>> "ls-tree" documentation seems to call it %(objectmode).
I can change it to 'objectmode' in a followup patch, if there are no
objections to this.
>> I think the idea is that "mode" being a too generic word can be used
>> for totally different purposes
My thinking was, no matter how generic or rare a name we choose, there
is always a chance
no matter how tiny, that the name will be in use in someone's .gitattributes.
But if people are ok with choosing something less generic
and have that become a 'reserved' attribute name and not have any
existing values in .gitattributes
take precedence I can do that.
I just don't know how git has historically balanced breaking existing
workflows vs easier/more reasonable
implementation/behavior.
>> But stepping back a bit,
>> such an application is likely marking selected few paths with the
>> attribute, and paths for which "mode" was "unset" are now given
>> these natural "mode"; it is inevitable to crash with such uses.
I'm confused. This does not match what I think is the current behavior
of my patch.
If "mode" was unset or removed for a path (meaning '<path> !mode' was
added to .gitattributes),
the code in my patch would respect that and not return the native mode.
It would return 'unspecified' or 'unset'. I have tests for these in
the patch, but if I've missed some
test cases please let me know.
>> Again, this has one hole, I think. Paths that are not mentioned
>> (not even with "!mode") would come to the function as ATTR__UNKNOWN
>> and trigger the fallback behaviour, even when other paths are given
>> end-user specified "mode" attribute values.
What you are describing sounds correct/what I intended.
So are you saying that the expected behavior is actually:
If the user sets 'mode' for 1+ paths in the repo, then the native mode
fallback should
NOT be used for all paths in the repo?
^ permalink raw reply
* [PATCH v3 2/3] update-index: add --show-index-version
From: Teng Long @ 2023-11-14 2:18 UTC (permalink / raw)
To: gitster; +Cc: git
In-Reply-To: <20230912193235.776292-3-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> @@ -1181,15 +1183,20 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>
> getline_fn = nul_term_line ? strbuf_getline_nul : strbuf_getline_lf;
> if (preferred_index_format) {
> - if (preferred_index_format < INDEX_FORMAT_LB ||
> - INDEX_FORMAT_UB < preferred_index_format)
> + if (preferred_index_format < 0) {
> + printf(_("%d\n"), the_index.version);
Maybe the "%d\n" shouldn't be translated? :)
Thanks.
^ permalink raw reply
* Re: [PATCH 02/13] Enable builds for z/OS.
From: Junio C Hamano @ 2023-11-14 1:47 UTC (permalink / raw)
To: brian m. carlson; +Cc: Haritha D via GitGitGadget, git, Haritha
In-Reply-To: <ZVKrWSv7JguKTSYw@tapette.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> I'd generally want to look at the commit message and understand the
> problem the code is trying to solve and then look at the code and think,
> "Oh, yes, this seems like the obvious and logical way to solve this
> problem," or at least think, "Oh, no, I think we should solve this
> problem in a different way," so I can help make a thoughtful review
> comment. Right now, I lack the information to have an informed opinion
> and so I can't provide any helpful feedback or analysis of the patches.
> ...
> I'd recommend a quick pass over the SubmittingPatches file, which is
> also available at https://git-scm.com/docs/SubmittingPatches. The
> sections on making separate commits for separate changes and describing
> changes well come to mind as places to focus.
>
> I know this may seem overwhelming and like I'm upset or disappointed;
> I'm definitely not. I'm very much interested in seeing Git available
> for more platforms, but right now it's too hard for me to reason about
> the changes for z/OS to provide helpful feedback, so I'm hoping you can
> send a fixed v2 that helps me (and everyone else) understand these
> changes better so you can get a helpful review.
All very good pieces of advice. I suspect we are missing some of
them from our SubmittingPatches or CodingGuidelines documents and
may want to add them there.
Thanks.
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Junio C Hamano @ 2023-11-14 1:40 UTC (permalink / raw)
To: Jeff King
Cc: Taylor Blau, Elijah Newren, git, Eric W. Biederman,
Patrick Steinhardt
In-Reply-To: <20231113220546.GB2065691@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I posted an alternative in response to Elijah; the general idea being to
> allow the usual object-lookup code to access the in-progress pack. That
> would keep us limited to a single pack.
If such a mechanism is done in a generic way, would we be able to
simplify fast-import a lot, I wonder? IIRC, it had quite a lot of
code to remember what it has written to its output to work around
the exact issue your alternative tries to solve. In fact, maybe we
could make fast-import a thin wrapper around the bulk checkin
infrastructure?
^ permalink raw reply
* Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof
From: Junio C Hamano @ 2023-11-14 1:36 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Jiří Hruška, git, Jeff King
In-Reply-To: <20231113212243.1495815-1-jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> Due to the nature of the bug and the fix, I do agree that it's hard to
> test and I would be OK with including the fix without associated tests.
Is this a bug on our side, or cURL library calling us when it should not?
Even if the latter, we should be prepared and the two liner fix you
suggested would be worth doing, but at the same time we should let
the cURL maintainer know if the latter is the case, and that is why
I am asking.
Thanks, both. Especially thanks, Jonathan, for excellent review
comments.
^ permalink raw reply
* Re: [PATCH] completion: add and use the __git_get_config_subsection helper function
From: Junio C Hamano @ 2023-11-14 1:08 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: git, Philippe Blain
In-Reply-To: <20231113222528.62771-1-szeder.dev@gmail.com>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> +# Lists all subsections in the given section which contain the given
> +# config variable, with the section and variable names removed.
> +__git_get_config_subsections ()
> +{
> + local section="$1" var="$2" i IFS=$'\n'
> + for i in $(__git config --name-only --get-regexp "^$section\..*\.$var$"); do
> + i=${i#$section.}
> + i=${i%.$var}
As this script is allowed bash-isms, I wondered if we can use
a single pattern substitution instead of two remove pre/suffix
pattern substitution, but I guess it would not work, and the above
is perfectly readable.
> + echo "$i"
As the subsection is designed to contain unbounded set of end-user
controlled names, we probably should do
printf "%s\n" "$i"
instead to protect us from interesting names (e.g. ones that begin
with a dash).
> + done
> +}
Interesting to see that we do not need to bother deduplicating the
output from here.
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index a7c3b4eb63..11ed83d0ed 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2130,6 +2130,19 @@ test_expect_success '__git_get_config_variables' '
> test_cmp expect actual
> '
>
> +test_expect_success '__git_get_config_subsections' '
> + cat >expect <<-\EOF &&
> + subsection-1
> + SubSection-2
> + sub.section.3
> + EOF
> + test_config interesting.subsection-1.name good &&
> + test_config Interesting.SubSection-2.Name good &&
> + test_config interesting.sub.section.3.name good &&
> + __git_get_config_subsections interesting name >actual &&
> + test_cmp expect actual
> +'
Good to see an uppercase character is used here ;-).
Thanks.
^ permalink raw reply
* Re: [PATCH] checkout: add config variable checkout.autoDetach
From: Junio C Hamano @ 2023-11-14 0:48 UTC (permalink / raw)
To: Phillip Wood; +Cc: Andy Koppe, git, pclouds
In-Reply-To: <0e37ee23-922c-4bbf-82c3-8f44e9216ab0@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
>> and good thing to help new users. I do not know how others react to
>> this kind of proliferation of configuration variables, but I do not
>> mind this particular variable existing.
>
> I'm a bit wary of having a config variable that could break
> scripts relying on the current behavior of "git checkout". As far
> as "git switch" goes I don't particularly mind this config
> variable though I'm not sure it is that hard to type "--detach"
> (especially with tab completion) and
I do not have much sympathy myself to scripts that are not being
defensive enough to write "--detach" explicitly, but I do understand
and share your concern as the project maintainer.
> ... I do worry that we're making the UI more complex each
> time we add something like this.
Thanks for saying this---this is exactly the kind of reaction I as
expecting to see.
>
> Best Wishes
>
> Phillip
^ permalink raw reply
* Re: [PATCH 02/13] Enable builds for z/OS.
From: Junio C Hamano @ 2023-11-14 0:48 UTC (permalink / raw)
To: Haritha D via GitGitGadget; +Cc: git, Haritha
In-Reply-To: <098b9ca8ece4fdce45a9b48e576b474ed81dced1.1699871056.git.gitgitgadget@gmail.com>
"Haritha D via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Subject: Re: [PATCH 02/13] Enable builds for z/OS.
Documentation/CodingGuidelines and Documentation/SubmittingPatches
would help here, I think.
> # Define SHELL_PATH to a POSIX shell if your /bin/sh is broken.
> #
> +# Define SHELL_PATH_FOR_SCRIPTS to a POSIX shell if your /bin/sh is broken.
The reason to exist for the _FOR_SCRIPTS variants is not justified
anywhere in the proposed log message.
The former should be sufficient, and our policy is to let the
builder specify exactly what binaries the build products depend on,
(instead of random $PATH interfere with the choice by using
"#!/bin/env tool" that also has to assume that everybody's "env" is
installed in "/bin").
This patch has too many #ifdefs in the primary codepaths. Your
porting strategy may need to be rethought. Our usual convention is
to encapsulate these platform differences as much as possible in
git-compat-util.h and platform specific files in compat/ directory.
> diff --git a/builtin/hash-object.c b/builtin/hash-object.c
> index 5ffec99dcea..b33b32ff977 100644
> --- a/builtin/hash-object.c
> +++ b/builtin/hash-object.c
> @@ -57,11 +57,39 @@ static void hash_fd(int fd, const char *type, const char *path, unsigned flags,
> maybe_flush_or_die(stdout, "hash to stdout");
> }
>
> +#ifdef __MVS__
> +# if (__CHARSET_LIB == 1)
> +# include <stdio.h>
> +# include <stdlib.h>
> +
> + int setbinaryfd(int fd)
> + {
> + attrib_t attr;
> + int rc;
Ahh, OK, I saw [03/13] first and was utterly confused by this thing.
Do not send in such a mess that introduces broken code in an early
step that you need to later say "oops that one was broken and I need
to fix it up with this patch". "rebase -i" is your friend to clean
up your mess into a logical progression to help readers better
understand what you wrote.
I'll stop here.
^ permalink raw reply
* Re: [PATCH 03/13] spaces and errors fix Handled git pipeline errors
From: Junio C Hamano @ 2023-11-14 0:38 UTC (permalink / raw)
To: Haritha D via GitGitGadget; +Cc: git, Haritha
In-Reply-To: <e31be0d764f47c21519016729259f8d74a53e21f.1699871056.git.gitgitgadget@gmail.com>
"Haritha D via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Subject: Re: [PATCH 03/13] spaces and errors fix Handled git pipeline errors
-ECANNOTPARSE. Perhaps Documentation/CodingGuidelines and
Documentation/SubmittingPatches may help?
> From: Haritha D <harithamma.d@ibm.com>
>
> This PR has fixes to enable build on z/OS
This is way under-explained. Your proposed log message should be
able to answer when somebody asks "Is anything broken in the
existing codebase to cause your build to fail, or is it your
compiler toolchain that is broken?" but the above does not help
understanding what and why you needed to fix at all.
> diff --git a/builtin/hash-object.c b/builtin/hash-object.c
> index b33b32ff977..9129658a37c 100644
> --- a/builtin/hash-object.c
> +++ b/builtin/hash-object.c
> @@ -62,8 +62,8 @@ static void hash_fd(int fd, const char *type, const char *path, unsigned flags,
> # include <stdio.h>
> # include <stdlib.h>
>
> - int setbinaryfd(int fd)
> - {
> +int setbinaryfd(int fd)
> +{
> attrib_t attr;
> int rc;
>
> @@ -74,7 +74,7 @@ static void hash_fd(int fd, const char *type, const char *path, unsigned flags,
>
> rc = __fchattr(fd, &attr, sizeof(attr));
> return rc;
> - }
> +}
> # endif
> #endif
No such function in our codebase. Are you fixing somebody else's
forked version of Git and we shouldn't even be looking at this
patch, perhaps?
> diff --git a/convert.c b/convert.c
> index 4f14ff6f1ed..17cc849efed 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -1315,15 +1315,28 @@ static struct attr_check *check;
>
> static const char* get_platform() {
> struct utsname uname_info;
> + char *result;
> + if(!uname_info.sysname)
> + {
> + result = (char *)malloc(strlen(uname_info.sysname)+1);
> + int index=0;
> + while(index <= strlen(uname_info.sysname))
> + {
> + *result = uname_info.sysname[index];
> + ++result;
> + ++index;
> + }
> + }
No such function in our codebase. I doubt these patches have much
relevance to this project?
I'll stop here.
^ permalink raw reply
* Re: [PATCH v2] ci: avoid running the test suite _twice_
From: Junio C Hamano @ 2023-11-14 0:24 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Jeff King, Johannes Schindelin
In-Reply-To: <pull.1613.v2.git.1699907108371.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> While this tries to fix a bug uncovered by js/doc-unit-tests, to avoid
> merge conflicts, this is based on ps/ci-gitlab.
Excellent choice of the base, as dropping the state is the right
thing to do regardless of the "unit-tests" thing (as Peff said),
and the refactoring done by ps/ci-gitlab topic is what makes the
problematic state=failed,slow,save spread wider and affects more
jobs.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH] ci: avoid running the test suite _twice_
From: Junio C Hamano @ 2023-11-13 23:55 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, Josh Steadmon, Phillip Wood,
git, Johannes Schindelin
In-Reply-To: <20231113184909.GB3838361@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I do have to wonder, though, as somebody who did not follow the
> unit-test topic closely: why are the unit tests totally separate from
> the rest of the suite? I would think we'd want them run from one or more
> t/t*.sh scripts. That would make bugs like this impossible, but also:
>
> 1. They'd be run via "make test", so developers don't have to remember
> to run them separately.
>
> 2. They can be run in parallel with all of the other tests when using
> "prove -j", etc.
Very good points. Josh?
^ permalink raw reply
* Re: commit-graph paranoia performance, was Re: [ANNOUNCE] Git v2.43.0-rc1
From: Junio C Hamano @ 2023-11-13 23:50 UTC (permalink / raw)
To: Jeff King; +Cc: Patrick Steinhardt, git, Karthik Nayak
In-Reply-To: <20231113205538.GA2028092@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Should we default GIT_COMMIT_GRAPH_PARANOIA to "0"? Yes, some operations
> might miss a breakage, but that is true of so much of Git. For day to
> day commands we generally assume that the repository is not corrupted,
> and avoid looking at any data we can. Other commands (like "commit-graph
> verify", but maybe others) would probably want to be more careful
> (either by checking this case explicitly, or by enabling the paranoia
> flag themselves).
I am obviously fine with that direction, as that was exactly the
stance I took when we discussed the topic on "rev-list --missing"
;-)
Patrick? Karthik?
^ permalink raw reply
* Re: [PATCH v4 0/3] t: improve compatibility with NixOS
From: Junio C Hamano @ 2023-11-13 23:42 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King
In-Reply-To: <ZVHM-0d7g45P3Uoj@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> Fair enough. I assumed that it would ease your workload instead of
> creating more work for you. But I'll keep in mind that it doesn't and
> refrain from doing this in the future.
Thanks.
With or without conflicts, basing your work on 'next' would not be a
good idea to begin with, as we never merge 'next' down to 'master'
(we only merge individual topics).
The following applies not specifically to you but to all
contributors.
What is helpful is in a tricky case is to start your topic branch
development at an appropriate base (often the tip of 'master', but
for a bugfix the tip of 'maint'), merge in other topics in flight
you depend on that are not in the target base (which limits what you
can depend on if you are writing a bugfix on 'maint'), and then
start building on top of the merge result. Then communicate how you
built this (artificial) base clearly in your cover letter.
Whether you have "other topics in flight" merged to your base or
not, creating a trial merge of your topic to seen to see how others'
work-in-progress may interact with your work would be a valuable way
to keep aware of what is cooking and how you will be affected.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Junio C Hamano @ 2023-11-13 23:27 UTC (permalink / raw)
To: Jeff King; +Cc: Arthur Chan via GitGitGadget, git, Arthur Chan
In-Reply-To: <20231113183509.GA3838361@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Nov 13, 2023 at 04:22:48PM +0000, Arthur Chan via GitGitGadget wrote:
>
>> + str = (char *)malloc(size + 1);
>> + if (!str)
>> + return 0;
>> + memcpy(str, data, size);
>> + str[size] = '\0';
>
> Is it important that we avoid calling die() if the malloc fails here?
>
> The usual way to write this in our code base is just:
>
> str = xmemdupz(data, size);
>
> It's not entirely a style thing; we sometimes audit the code base
> looking for computations on malloc sizes (for integer overflows) as well
> as sites that should be using xmalloc and are not. Obviously we can
> exclude oss-fuzz/ from such audits, but if there's no reason not to
> prefer our usual style, it's one less thing to worry about.
Good point. Thanks.
^ permalink raw reply
* Re: [PATCH v2] fuzz: add new oss-fuzz fuzzer for date.c / date.h
From: Junio C Hamano @ 2023-11-13 23:27 UTC (permalink / raw)
To: Arthur Chan via GitGitGadget; +Cc: git, Arthur Chan
In-Reply-To: <pull.1612.v2.git.1699892568344.gitgitgadget@gmail.com>
"Arthur Chan via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
It is somewhat annoying that everybody has to repeat this twice
here, but it is not your fault X-<.
> +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
> +{
> + int local;
> + int num;
> + uint16_t tz;
tz offset can be negative, so uint16_t is not appropriate. See
date.c:gm_time_t() that is eventually called from show_date().
> + char *str;
> + timestamp_t ts;
> + enum date_mode_type dmtype;
> + struct date_mode *dm;
> +
> + if (size <= 4)
> + /*
> + * we use the first byte to fuzz dmtype and local,
> + * then the next three bytes to fuzz tz offset,
> + * and the remainder (at least one byte) is fed
> + * as end-user input to approxidate_careful().
> + */
> + return 0;
> +
> + local = !!(*data & 0x10);
> + dmtype = (enum date_mode_type)(*data % DATE_UNIX);
> + if (dmtype == DATE_STRFTIME)
> + /*
> + * Currently DATE_STRFTIME is not supported.
> + */
> + return 0;
There is an off-by-one error above, as modulo DATE_UNIX will never
yield DATE_UNIX. Presumably we could do something silly like
tmp = *data % DATE_UNIX;
if (DATE_STRFTIME <= tmp)
tmp++;
dmtime = (enum date_mode_type)tmp;
to pick values from [0..DATE_UNIX) and then shift everything above
DATE_STRFTIME by one to create a hole there and fill DATE_UNIX at
the same time, without wasting a sample by returning.
> + data++;
> + size--;
> +
> + tz = *data++;
> + tz = (tz << 8) | *data++;
> + tz = (tz << 8) | *data++;
> + size -= 3;
If your tz is 16-bit wide, then we do not have to eat three bytes
here, do we?
You never answered my question on your intention. Is "tz"
considered attacker controlled (and needs to be fuzzed including
invalid values)?
> + str = (char *)malloc(size + 1);
> + if (!str)
> + return 0;
> + memcpy(str, data, size);
> + str[size] = '\0';
> +
> + ts = approxidate_careful(str, &num);
> + free(str);
> +
> + dm = date_mode_from_type(dmtype);
> + dm->local = local;
> + show_date(ts, (int16_t)tz, dm);
> +
> + date_mode_release(dm);
> +
> + return 0;
> +}
>
> base-commit: dadef801b365989099a9929e995589e455c51fed
Thanks.
^ permalink raw reply
* [PATCH v2] glossary: add definitions for dereference & peel
From: Victoria Dye via GitGitGadget @ 2023-11-13 23:17 UTC (permalink / raw)
To: git; +Cc: ps, Kristoffer Haugsbakk, Victoria Dye, Victoria Dye
In-Reply-To: <pull.1610.git.1699574277143.gitgitgadget@gmail.com>
From: Victoria Dye <vdye@github.com>
Add 'gitglossary' definitions for "dereference" (as it used for both symrefs
and objects) and "peel". These terms are used in options and documentation
throughout Git, but they are not clearly defined anywhere and the behavior
they refer to depends heavily on context. Provide explicit definitions to
clarify existing documentation to users and help contributors to use the
most appropriate terminology possible in their additions to Git.
Update other definitions in the glossary that use the term "dereference" to
link to 'def_dereference'.
Signed-off-by: Victoria Dye <vdye@github.com>
---
glossary: add definitions for dereference & peel
As promised in [1], this patch adds definitions for "peel" and
"dereference" in the glossary, based on how they're currently used
throughout Git. As a result, the definitions are somewhat broad
(although I did my best to explicitly describe the different contexts in
which they're used). My hope is that this will at least reduce confusion
around this terminology. These definitions can also serve as a starting
point if, in the future, another contributor wants to deprecate certain
usages of these terms to make them less ambiguous.
* Victoria
[1]
https://lore.kernel.org/git/21dfe606-39f5-4154-aaa4-695e5f6f784d@github.com/
Changes since V1
================
* Removed references to "peeling" a commit; the updated definition
discusses "peeling" only in the context of tags.
* Added a cross-link from "dereference" to "peel" (one already existed
for "peel" to "dereference").
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1610%2Fvdye%2Fvdye%2Fglossary-peel-dereference-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1610/vdye/vdye/glossary-peel-dereference-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1610
Range-diff vs v1:
1: e40fc3e5e04 ! 1: 4d9e0d7fc81 glossary: add definitions for dereference & peel
@@ Documentation/glossary-content.txt: to point at the new commit.
+<<def_object,object>> a tag points at. Tags are recursively dereferenced by
+repeating the operation on the result object until the result has either a
+specified <<def_object_type,object type>> (where applicable) or any non-"tag"
-+object type.
++object type. A synonym for "recursive dereference" in the context of tags is
++"<<def_peel,peel>>".
++
+Referring to a <<def_commit_object,commit object>>: the action of accessing
+the commit's tree object. Commits cannot be dereferenced recursively.
@@ Documentation/glossary-content.txt: exclude;;
parents.
+[[def_peel]]peel::
-+ Synonym for object <<def_dereference,dereference>>. Most commonly used
-+ in the context of tags, where it refers to the process of recursively
-+ dereferencing a <<def_tag_object,tag object>> until the result object's
-+ <<def_object_type,type>> is something other than "tag".
++ The action of recursively <<def_dereference,dereferencing>> a
++ <<def_tag_object,tag object>>.
+
[[def_pickaxe]]pickaxe::
The term <<def_pickaxe,pickaxe>> refers to an option to the diffcore
Documentation/glossary-content.txt | 49 +++++++++++++++++++++---------
1 file changed, 35 insertions(+), 14 deletions(-)
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 65c89e7b3eb..59d8ab85721 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -98,9 +98,8 @@ to point at the new commit.
revision.
[[def_commit-ish]]commit-ish (also committish)::
- A <<def_commit_object,commit object>> or an
- <<def_object,object>> that can be recursively dereferenced to
- a commit object.
+ A <<def_commit_object,commit object>> or an <<def_object,object>> that
+ can be recursively <<def_dereference,dereferenced>> to a commit object.
The following are all commit-ishes:
a commit object,
a <<def_tag_object,tag object>> that points to a commit
@@ -125,6 +124,25 @@ to point at the new commit.
dangling object has no references to it from any
reference or <<def_object,object>> in the <<def_repository,repository>>.
+[[def_dereference]]dereference::
+ Referring to a <<def_symref,symbolic ref>>: the action of accessing the
+ <<def_ref,reference>> pointed at by a symbolic ref. Recursive
+ dereferencing involves repeating the aforementioned process on the
+ resulting ref until a non-symbolic reference is found.
++
+Referring to a <<def_tag_object,tag object>>: the action of accessing the
+<<def_object,object>> a tag points at. Tags are recursively dereferenced by
+repeating the operation on the result object until the result has either a
+specified <<def_object_type,object type>> (where applicable) or any non-"tag"
+object type. A synonym for "recursive dereference" in the context of tags is
+"<<def_peel,peel>>".
++
+Referring to a <<def_commit_object,commit object>>: the action of accessing
+the commit's tree object. Commits cannot be dereferenced recursively.
++
+Unless otherwise specified, "dereferencing" as it used in the context of Git
+commands or protocols is implicitly recursive.
+
[[def_detached_HEAD]]detached HEAD::
Normally the <<def_HEAD,HEAD>> stores the name of a
<<def_branch,branch>>, and commands that operate on the
@@ -444,6 +462,10 @@ exclude;;
of the logical predecessor(s) in the line of development, i.e. its
parents.
+[[def_peel]]peel::
+ The action of recursively <<def_dereference,dereferencing>> a
+ <<def_tag_object,tag object>>.
+
[[def_pickaxe]]pickaxe::
The term <<def_pickaxe,pickaxe>> refers to an option to the diffcore
routines that help select changes that add or delete a given text
@@ -620,12 +642,11 @@ The most notable example is `HEAD`.
copies of) commit objects of the contained submodules.
[[def_symref]]symref::
- Symbolic reference: instead of containing the <<def_SHA1,SHA-1>>
- id itself, it is of the format 'ref: refs/some/thing' and when
- referenced, it recursively dereferences to this reference.
- '<<def_HEAD,HEAD>>' is a prime example of a symref. Symbolic
- references are manipulated with the linkgit:git-symbolic-ref[1]
- command.
+ Symbolic reference: instead of containing the <<def_SHA1,SHA-1>> id
+ itself, it is of the format 'ref: refs/some/thing' and when referenced,
+ it recursively <<def_dereference,dereferences>> to this reference.
+ '<<def_HEAD,HEAD>>' is a prime example of a symref. Symbolic references
+ are manipulated with the linkgit:git-symbolic-ref[1] command.
[[def_tag]]tag::
A <<def_ref,ref>> under `refs/tags/` namespace that points to an
@@ -661,11 +682,11 @@ The most notable example is `HEAD`.
<<def_tree,tree>> is equivalent to a <<def_directory,directory>>.
[[def_tree-ish]]tree-ish (also treeish)::
- A <<def_tree_object,tree object>> or an <<def_object,object>>
- that can be recursively dereferenced to a tree object.
- Dereferencing a <<def_commit_object,commit object>> yields the
- tree object corresponding to the <<def_revision,revision>>'s
- top <<def_directory,directory>>.
+ A <<def_tree_object,tree object>> or an <<def_object,object>> that can
+ be recursively <<def_dereference,dereferenced>> to a tree object.
+ Dereferencing a <<def_commit_object,commit object>> yields the tree
+ object corresponding to the <<def_revision,revision>>'s top
+ <<def_directory,directory>>.
The following are all tree-ishes:
a <<def_commit-ish,commit-ish>>,
a tree object,
base-commit: dadef801b365989099a9929e995589e455c51fed
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 02/13] Enable builds for z/OS.
From: brian m. carlson @ 2023-11-13 23:03 UTC (permalink / raw)
To: Haritha D via GitGitGadget; +Cc: git, Haritha
In-Reply-To: <098b9ca8ece4fdce45a9b48e576b474ed81dced1.1699871056.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 10819 bytes --]
On 2023-11-13 at 10:24:04, Haritha D via GitGitGadget wrote:
> From: Haritha D <harithamma.d@ibm.com>
>
> This commit enables git to build on z/OS.
> It takes advantage of enahanced ASCII
> services on z/OS to auto-convert input
> files to ASCII
In general, we don't want to convert files to ASCII, since we assume
text files are UTF-8 unless otherwise specified. If you're suggesting
that your system is normally EBCDIC, changing that should be an initial
piece of setup code or something used in `xopen` on your platform so it
applies everywhere with minimal changes.
> It also adds support for
> [platform]-working-tree-encoding.
> Platform is substituted with uname_info.sysname,
> so it will only apply to the given platform.
I think this is going to need to be a separate patch and feature with
its own explanation of why it's valuable.
> Also adds support for scripts that are not in
> standard locations so that /bin/env bash
> can be specified.
> Signed-off-by: Harithamma D <harithamma.d@ibm.com>
> ---
> Makefile | 21 +++++++++---
> builtin.h | 3 ++
> builtin/archive.c | 6 ++++
> builtin/hash-object.c | 28 +++++++++++++++
> combine-diff.c | 4 +++
> config.c | 7 ++++
> configure.ac | 3 ++
> convert.c | 44 ++++++++++++++++++++----
> copy.c | 3 ++
> diff.c | 11 ++++++
> entry.c | 26 ++++++++++++++
> environment.c | 3 ++
> git-compat-util.h | 8 +++++
> negotiator/default.c | 4 +--
> negotiator/noop.c | 4 +--
> negotiator/skipping.c | 4 +--
> object-file.c | 80 ++++++++++++++++++++++++++++++++++++++++++-
> read-cache.c | 3 ++
> utf8.c | 11 ++++++
> 19 files changed, 255 insertions(+), 18 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 9c6a2f125f8..30aa76da4f4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -20,6 +20,8 @@ include shared.mak
> #
> # Define SHELL_PATH to a POSIX shell if your /bin/sh is broken.
> #
> +# Define SHELL_PATH_FOR_SCRIPTS to a POSIX shell if your /bin/sh is broken.
> +#
> # Define SANE_TOOL_PATH to a colon-separated list of paths to prepend
> # to PATH if your tools in /usr/bin are broken.
> #
> @@ -215,6 +217,8 @@ include shared.mak
> #
> # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).
> #
> +# Define PERL_PATH_FOR_SCRIPTS to a Perl binary if your /usr/bin/perl is broken.
You will probably want to explain in your commit message why these two
are required to be different from the standard values and explain here
what the relevant difference is so that users can set them
appropriately.
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 90761fdfee0..53ec794356f 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -14,6 +14,12 @@
> static void create_output_file(const char *output_file)
> {
> int output_fd = xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> +#ifdef __MVS__
> + #if (__CHARSET_LIB == 1)
> + if (setbinaryfd(output_fd))
> + die_errno(_("could not tag archive file '%s'"), output_file);
> + #endif
> +#endif
This would be better to place in `xopen` itself so that all files are
correctly configured. That would do well as its own patch, and you'd
want to explain well in the commit message what this function does, why
it's necessary, and what the consequences of not using it are, as well
as any alternatives that you've rejected.
> if (output_fd != 1) {
> if (dup2(output_fd, 1) < 0)
> die_errno(_("could not redirect output"));
> diff --git a/builtin/hash-object.c b/builtin/hash-object.c
> index 5ffec99dcea..b33b32ff977 100644
> --- a/builtin/hash-object.c
> +++ b/builtin/hash-object.c
> @@ -57,11 +57,39 @@ static void hash_fd(int fd, const char *type, const char *path, unsigned flags,
> maybe_flush_or_die(stdout, "hash to stdout");
> }
>
> +#ifdef __MVS__
> +# if (__CHARSET_LIB == 1)
> +# include <stdio.h>
> +# include <stdlib.h>
We typically don't include the standard headers here. Instead, they're
included by git-compat-util.h because on some systems they have to be
included in a certain order with certain options. If you include that
header instead at the top of the file, or one of the headers that
includes it, then typically that should do the right thing.
> + int setbinaryfd(int fd)
> + {
> + attrib_t attr;
> + int rc;
> +
> + memset(&attr, 0, sizeof(attr));
> + attr.att_filetagchg = 1;
> + attr.att_filetag.ft_ccsid = FT_BINARY;
> + attr.att_filetag.ft_txtflag = 0;
> +
> + rc = __fchattr(fd, &attr, sizeof(attr));
> + return rc;
> + }
> +# endif
> +#endif
I would think a comment explaining what this function does and why it's
necessary would be appropriate, since it's not in POSIX and isn't
typically necessary on POSIX systems.
> diff --git a/combine-diff.c b/combine-diff.c
> index f90f4424829..73445a517c7 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1082,6 +1082,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
> ssize_t done;
> int is_file, i;
>
> +#ifdef __MVS__
> + __disableautocvt(fd);
> +#endif
Again, if this can be centralized, it should be, and it should be
explained well in the commit message. I'm uncertain what it does or
what value it provides.
> diff --git a/config.c b/config.c
> index f9a1cca4e8a..37c124a37c0 100644
> --- a/config.c
> +++ b/config.c
> @@ -1521,6 +1521,13 @@ static int git_default_core_config(const char *var, const char *value,
> return 0;
> }
>
> + #ifdef __MVS__
> + if (!strcmp(var, "core.ignorefiletags")) {
> + ignore_file_tags = git_config_bool(var, value);
> + return 0;
> + }
> + #endif
This should also live in its own patch and the commit message should
explain what it does. We'd also want it to be documented in the config
options in the Documentation directory.
> diff --git a/convert.c b/convert.c
> index a8870baff36..4f14ff6f1ed 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -377,12 +377,15 @@ static int check_roundtrip(const char *enc_name)
> static const char *default_encoding = "UTF-8";
>
> static int encode_to_git(const char *path, const char *src, size_t src_len,
> - struct strbuf *buf, const char *enc, int conv_flags)
> + struct strbuf *buf, const char *enc, enum convert_crlf_action attr_action, int conv_flags)
> {
> char *dst;
> size_t dst_len;
> int die_on_error = conv_flags & CONV_WRITE_OBJECT;
>
> + if (attr_action == CRLF_BINARY) {
> + return 0;
> + }
I'm pretty sure this is a change in behaviour from what we had before.
It should live in its own patch, with an explanation in the commit
message why it's a compelling and correct change overall, and with
suitable tests.
> /*
> * No encoding is specified or there is nothing to encode.
> * Tell the caller that the content was not modified.
> @@ -403,6 +406,11 @@ static int encode_to_git(const char *path, const char *src, size_t src_len,
> return 0;
>
> trace_encoding("source", path, enc, src, src_len);
> +#ifdef __MVS__
> + // Don't convert ISO8859-1 on z/OS
> + if (strcasecmp("ISO8859-1", enc) == 0)
> + return 0;
> +#endif
This definitely needs explanation in the commit message and should
probably be its own patch, explaining why z/OS has this compelling need
to not convert ISO8859-1. Note that ISO8859-1 is not the same as "no
binary conversion", since it doesn't include many control codes.
Note that if, as it says later on, this really means "UTF-8", that's a
platform wart you'd want to paper over in a file in the compat code. In
general, the compat directory is a good place to put anything that your
platform needs specifically.
> +static const char* get_platform() {
> + struct utsname uname_info;
> +
> + if (uname(&uname_info))
> + die(_("uname() failed with error '%s' (%d)\n"),
> + strerror(errno),
> + errno);
> +
> + if (!strcmp(uname_info.sysname, "OS/390"))
> + return "zos";
> + return uname_info.sysname;
> +}
This is definitely a new feature, and I'm not sure why it's necessary or
useful. I suspect there's something about z/OS that makes it valuable,
but I don't know what it is since the commit message doesn't tell me.
I'm also not sure that these values will be correct on Windows.
I think I could go on to make similar comments about the rest of this
series. I'm not opposed to seeing z/OS changes come in, but you've
amalgamated at least a half-dozen separate patches into one and haven't
explained them very thoroughly in the commit message.
I'd generally want to look at the commit message and understand the
problem the code is trying to solve and then look at the code and think,
"Oh, yes, this seems like the obvious and logical way to solve this
problem," or at least think, "Oh, no, I think we should solve this
problem in a different way," so I can help make a thoughtful review
comment. Right now, I lack the information to have an informed opinion
and so I can't provide any helpful feedback or analysis of the patches.
When you're adding new features or fixing bugs, we'll also want tests
for those cases to help us avoid regressing that code in the future.
Even if we don't normally run the testsuite on z/OS, at least _you_ will
notice that the tests have failed and then we'll be able to address the
bugs in a timely manner.
I also noted that there were some fixup commits later on in the series
that address whitespace issues. Typically, we'd want to squash those in
to the earlier patches. Nobody expects perfection, but squashing errors
into earlier patches helps us keep the history neat and let us pretend
like you never made those errors at all. It also lets tools like git
blame and git bisect work more nicely for users.
I'd recommend a quick pass over the SubmittingPatches file, which is
also available at https://git-scm.com/docs/SubmittingPatches. The
sections on making separate commits for separate changes and describing
changes well come to mind as places to focus.
I know this may seem overwhelming and like I'm upset or disappointed;
I'm definitely not. I'm very much interested in seeing Git available
for more platforms, but right now it's too hard for me to reason about
the changes for z/OS to provide helpful feedback, so I'm hoping you can
send a fixed v2 that helps me (and everyone else) understand these
changes better so you can get a helpful review.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Taylor Blau @ 2023-11-13 22:34 UTC (permalink / raw)
To: Jeff King
Cc: Elijah Newren, git, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <20231113220254.GA2065691@coredump.intra.peff.net>
On Mon, Nov 13, 2023 at 05:02:54PM -0500, Jeff King wrote:
> On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:
>
> > This is unsafe; the object may need to be read later within the same
> > merge. [...]
> >
> > I think (untested) that you could fix this by creating two packs
> > instead of just one. In particular, add a call to
> > flush_odb_transcation() after the "redo_after_renames" block in
> > merge_ort_nonrecursive_internal().
>
> It might not be too hard to just let in-process callers access the
> objects we've written. A quick and dirty patch is below, which works
> with the test you suggested (the test still fails because it finds a
> conflict, but it gets past the "woah, I can't find that sha1" part).
That's a very slick idea, and I think that this series has some legs to
stand on now as a result.
There are a few of interesting conclusions we can draw here:
1. This solves the recursive merge problem that Elijah mentioned
earlier where we could generate up to 2^N packs (where N is the
maximum depth of the recursive merge).
2. This also solves the case where the merge-ort code needs to read an
object that it wrote earlier on in the same process without having
to flush out intermediate packs. So in the best case we need only a
single pack (instead of two).
3. This also solves the 'replay' issue, *and* allows us to avoid the
tmp-objdir thing there entirely, since we can simulate object reads
in the bulk-checkin pack.
So I think that this is a direction worth pursuing.
In terms of making those lookups faster, I think that what you'd want is
an oidmap. The overhead is slightly unfortunate, but I think that any
other solution which requires keeping the written array in sorted order
would in practice be more expensive as you have to constantly reallocate
and copy portions of the array around to maintain its invariant.
> I don't know if that is sufficient, though. Would other spawned
> processes (hooks, external merge drivers, and so on) need to be able to
> see these objects, too?
Interesting point. In theory those processes could ask about newly
created objects, and if they were spawned before the bulk-checkin pack
was flushed, those lookups would fail.
But that requires that, e.g. for hooks, that we know a-priori the object
ID of some newly-written objects. If you wanted to make those lookups
succeed, I think there are a couple of options:
- teach child-processes about the bulk-checkin pack, and let them
perform the same fake lookup
- flush (but do not close) the bulk-checkin transaction
In any event, I think that this is a sufficiently rare and niche case
that we'd be OK to declare that you should not expect the above
scenarios to work when using `--write-pack`. If someone does ask for
that feature in the future, we could implement it relatively painlessly
using one of the above options.
Thanks,
Taylor
^ permalink raw reply
* [PATCH] completion: add and use the __git_get_config_subsection helper function
From: SZEDER Gábor @ 2023-11-13 22:25 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Junio C Hamano, SZEDER Gábor
Our Bash completion script recently learned to complete configured
trailer key aliases for 'git config --trailer=<TAB>', but the helper
function extracting the key alias from 'git config's output, i.e. the
subsection from 'trailer.*.key', ended up more complex than necessary,
with considerable overhead from executing four external processes in a
pipe.
Replace those commands in the pipe with a simple shell loop using only
a pair of parameter expansions and a builtin 'echo', which is easier
to understand and should perform better (I assume that users don't
have that many subsections in any particular section to make the
processing with an external process (let alone four) worth it).
And while at it, let's extract this loop into a generic
__git_get_config_subsections() helper function, as it might be useful
elsewhere in the future as well (at the moment it isn't, AFAICT).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
contrib/completion/git-completion.bash | 14 +++++++++++++-
t/t9902-completion.sh | 13 +++++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 13a39ebd2e..34bbb66f85 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1148,6 +1148,18 @@ __git_get_config_variables ()
done
}
+# Lists all subsections in the given section which contain the given
+# config variable, with the section and variable names removed.
+__git_get_config_subsections ()
+{
+ local section="$1" var="$2" i IFS=$'\n'
+ for i in $(__git config --name-only --get-regexp "^$section\..*\.$var$"); do
+ i=${i#$section.}
+ i=${i%.$var}
+ echo "$i"
+ done
+}
+
__git_pretty_aliases ()
{
__git_get_config_variables "pretty"
@@ -1681,7 +1693,7 @@ __git_untracked_file_modes="all no normal"
__git_trailer_tokens ()
{
- __git config --name-only --get-regexp '^trailer\..*\.key$' | cut -d. -f 2- | rev | cut -d. -f2- | rev
+ __git_get_config_subsections trailer key
}
_git_commit ()
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a7c3b4eb63..11ed83d0ed 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2130,6 +2130,19 @@ test_expect_success '__git_get_config_variables' '
test_cmp expect actual
'
+test_expect_success '__git_get_config_subsections' '
+ cat >expect <<-\EOF &&
+ subsection-1
+ SubSection-2
+ sub.section.3
+ EOF
+ test_config interesting.subsection-1.name good &&
+ test_config Interesting.SubSection-2.Name good &&
+ test_config interesting.sub.section.3.name good &&
+ __git_get_config_subsections interesting name >actual &&
+ test_cmp expect actual
+'
+
test_expect_success '__git_pretty_aliases' '
cat >expect <<-EOF &&
author
--
2.43.0.rc1.528.g8f9d60d041
^ permalink raw reply related
* Re: [PATCH 1/1] attr: add native file mode values support
From: Junio C Hamano @ 2023-11-13 22:22 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: Joanna Wang, git
In-Reply-To: <20231113165031.GA28778@tb-raspi4>
Torsten Bögershausen <tboegi@web.de> writes:
> On Sat, Nov 11, 2023 at 04:03:08AM +0000, Joanna Wang wrote:
>
> Some thoughts and comments inline...
>
>> Gives all paths inherent 'mode' attribute values based on the paths'
>> modes (one of 100644, 100755, 120000, 040000, 160000). Users may use
>> this feature to filter by file types. For example a pathspec such as
>> ':(attr:mode=160000)' could filter for submodules without needing
>
> My spontanous feeling is that filetype may be another choice:
>> ':(attr:filetype=160000)' could filter for submodules without needing
I do agree that "mode" invites "mode of what???" reaction, and that
a term that narrows the scope would be preferrable. "Filemode" is a
bit questionable, though, as we give this permbits to non-files like
submodules. "ls-tree" documentation seems to call it %(objectmode).
> And having written this, we can think using something borrowed from
> `find . -type f`
>
> :(attr:filetype=f)' or :(attr:filetype=x)' (for executable)
This would not work for submodules, though. Naively one might want
to abuse 'd' but I suspect we would eventually want to be able to
give the mode bits to an out-of-cone directory storeed in the index
as a tree in a cone-mode sparse checkout, which would be 040000,
which deserves 'd' more than submodules.
> But then I missed the point why we need an attribute here?
> The mode is already defined by the the file system (and Git),
> is there a special reason that the user can define or re-define the
> value here ?
I think the idea is that "mode" being a too generic word can be used
for totally different purposes in existing projects and the addition
did not want to disturb their own use. But stepping back a bit,
such an application is likely marking selected few paths with the
attribute, and paths for which "mode" was "unset" are now given
these natural "mode"; it is inevitable to crash with such uses. If
we want to introduce "native" attributes of this kind, we would
probably need to carve out namespaces a bit more clearaly.
> May be there is, when working with pathspec.
> But then "pathspec=" could be a better construction.
> Since "mode" could make a reader think that Git does somewhat with the file
> when checking out.
> My personal hope reading "mode=100755" in .gitattributes would
> be that Git makes it executable when checking out, if if it is
> recorded in Git as 100644, probably coming from a file-system that
> doesn't support the executable bit in a Unix way.
That is not the intended way this attribute is to be used. Perhaps
we should make it an error (or ignored) when certain built-in/native
attributes are seen in the attribute file, but again that takes some
namespace carved out to avoid crashing with end-user names.
>> If there is any existing mode attribute for a path (e.g. there is
>> !mode, -mode, mode, mode=<value> in .gitattributes) that setting will
>> take precedence over the native mode value.
Again, this has one hole, I think. Paths that are not mentioned
(not even with "!mode") would come to the function as ATTR__UNKNOWN
and trigger the fallback behaviour, even when other paths are given
end-user specified "mode" attribute values.
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Jeff King @ 2023-11-13 22:05 UTC (permalink / raw)
To: Taylor Blau
Cc: Elijah Newren, git, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <ZU7X3N/rqCK/Y9cj@nand.local>
On Fri, Nov 10, 2023 at 08:24:44PM -0500, Taylor Blau wrote:
> > This does mean that for a recursive merge, that you'll get up to 2*N
> > packfiles, where N is the depth of the recursive merge.
>
> We definitely want to avoid that ;-). I think there are a couple of
> potential directions forward here, but the most promising one I think is
> due to Johannes who suggests that we write loose objects into a
> temporary directory with a replace_tmp_objdir() call, and then repack
> that side directory before migrating a single pack back into the main
> object store.
I posted an alternative in response to Elijah; the general idea being to
allow the usual object-lookup code to access the in-progress pack. That
would keep us limited to a single pack.
It _might_ be a terrible idea. E.g., if you write a non-bulk object that
references a bulk one, then that non-bulk one is broken from the
perspective of other processes (until the bulk checkin is flushed). But
I think we'd always be writing to one or the other here, never
interleaving?
-Peff
^ permalink raw reply
* Re: [PATCH v5 5/5] builtin/merge-tree.c: implement support for `--write-pack`
From: Jeff King @ 2023-11-13 22:02 UTC (permalink / raw)
To: Elijah Newren
Cc: Taylor Blau, git, Eric W. Biederman, Junio C Hamano,
Patrick Steinhardt
In-Reply-To: <CABPp-BEfy9VOvimP9==ry_rZXu=metOQ8s=_-XiG_Pdx9c06Ww@mail.gmail.com>
On Fri, Nov 10, 2023 at 03:51:18PM -0800, Elijah Newren wrote:
> This is unsafe; the object may need to be read later within the same
> merge. [...]
>
> I think (untested) that you could fix this by creating two packs
> instead of just one. In particular, add a call to
> flush_odb_transcation() after the "redo_after_renames" block in
> merge_ort_nonrecursive_internal().
It might not be too hard to just let in-process callers access the
objects we've written. A quick and dirty patch is below, which works
with the test you suggested (the test still fails because it finds a
conflict, but it gets past the "woah, I can't find that sha1" part).
I don't know if that is sufficient, though. Would other spawned
processes (hooks, external merge drivers, and so on) need to be able to
see these objects, too?
The patch teaches the packfile code about the special bulk checkin pack.
It might be cleaner to invert it, and just have the bulk checkin code
register a magic packed_git (it would need to fake the .idx somehow).
diff --git a/bulk-checkin.c b/bulk-checkin.c
index bd6151ba3c..566fc36e68 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -30,6 +30,8 @@ static struct bulk_checkin_packfile {
struct pack_idx_entry **written;
uint32_t alloc_written;
uint32_t nr_written;
+
+ struct packed_git *fake_packed_git;
} bulk_checkin_packfile;
static void finish_tmp_packfile(struct strbuf *basename,
@@ -82,6 +84,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
clear_exit:
free(state->written);
+ free(state->fake_packed_git);
memset(state, 0, sizeof(*state));
strbuf_release(&packname);
@@ -530,3 +533,37 @@ void end_odb_transaction(void)
flush_odb_transaction();
}
+
+static struct packed_git *fake_packed_git(struct bulk_checkin_packfile *state)
+{
+ struct packed_git *p = state->fake_packed_git;
+ if (!p) {
+ FLEX_ALLOC_STR(p, pack_name, "/fake/in-progress.pack");
+ state->fake_packed_git = p;
+ p->pack_fd = state->f->fd;
+ p->do_not_close = 1;
+ }
+
+ hashflush(state->f);
+ p->pack_size = state->f->total; /* maybe add 20 to simulate trailer? */
+
+ return p;
+}
+
+int bulk_checkin_pack_entry(const struct object_id *oid, struct pack_entry *e)
+{
+ size_t i;
+ /*
+ * this really ought to have some more efficient data structure for
+ * lookup; ditto for the existing already_written()
+ */
+ for (i = 0; i < bulk_checkin_packfile.nr_written; i++) {
+ struct pack_idx_entry *p = bulk_checkin_packfile.written[i];
+ if (oideq(&p->oid, oid)) {
+ e->p = fake_packed_git(&bulk_checkin_packfile);
+ e->offset = p->offset;
+ return 0;
+ }
+ }
+ return -1;
+}
diff --git a/bulk-checkin.h b/bulk-checkin.h
index 89786b3954..153fe87c06 100644
--- a/bulk-checkin.h
+++ b/bulk-checkin.h
@@ -44,4 +44,7 @@ void flush_odb_transaction(void);
*/
void end_odb_transaction(void);
+struct pack_entry;
+int bulk_checkin_pack_entry(const struct object_id *oid, struct pack_entry *e);
+
#endif
diff --git a/packfile.c b/packfile.c
index 9cc0a2e37a..05194b1d9b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -23,6 +23,7 @@
#include "commit-graph.h"
#include "pack-revindex.h"
#include "promisor-remote.h"
+#include "bulk-checkin.h"
char *odb_pack_name(struct strbuf *buf,
const unsigned char *hash,
@@ -2045,6 +2046,9 @@ int find_pack_entry(struct repository *r, const struct object_id *oid, struct pa
struct list_head *pos;
struct multi_pack_index *m;
+ if (!bulk_checkin_pack_entry(oid, e))
+ return 1;
+
prepare_packed_git(r);
if (!r->objects->packed_git && !r->objects->multi_pack_index)
return 0;
^ permalink raw reply related
* Re: [PATCH] remote-curl: avoid hang if curl asks for more data after eof
From: Jonathan Tan @ 2023-11-13 21:22 UTC (permalink / raw)
To: Jiří Hruška; +Cc: Jonathan Tan, git, Jeff King
In-Reply-To: <CAGE_+C6DJMAO0bj5QHoKBBV3gMEMtZ-ajJ9ZnDGYq6eorr-oig@mail.gmail.com>
Jiří Hruška <jirka@fud.cz> writes:
> It has been observed that under some circumstances, libcurl can call
> our `CURLOPT_READFUNCTION` callback `rpc_out()` again even after
> already getting a return value of zero (EOF) back once before.
>
> This results in `rpc_read_from_out()` trying to read more from the
> child process pipe, because `rpc->flush_read_but_not_sent` is reset to
> false already the first time EOF is returned, and then the whole
> operation gets stuck - the child process is already trying to read
> a response back and will not write anything to the output pipe anymore,
> while the parent/remote process is now blocked waiting to read more too
> and never even finishes sending the request.
Ah, thanks for finding this bug. It sounds worthwhile to make Git more
resilient in this situation.
I'll just make some preliminary comments.
> This commit addresses the bug with the following changes:
[snip]
This seems like a long list of changes, when from the description of the
bug above, I would have assumed it sufficient to record somewhere when
the CURLOPT_READFUNCTION callback returns zero, and then always return
zero after that if the callback is ever re-invoked. If this is indeed
not sufficient, we should describe why.
Also, if multiple changes are needed, please split them into several
commits.
> A trivial solution would be to just take the line which resets the flag
>
> /*
> * The line length either does not need to be sent at
> * all or has already been completely sent. Now we can
> * return 0, indicating EOF, meaning that the flush has
> * been fully sent.
> */
> - rpc->flush_read_but_not_sent = 0;
> return 0;
>
> from rpc_out() and reset it only in post_rpc(), before the next time a large
> request is being sent out and rpc_out() will go into play again:
>
> if (large_request) {
> ...
> rpc->initial_buffer = 1;
> + rpc->flush_read_but_not_sent = 0;
> curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
>
> This way the CURLOPT_READFUNCTION would be returning zeros at the end of the
> upload as long as needed, just like fread() at the end of a real file.
>
> Hence, the bug could be fixed with just that two-lines change.
Ah, you describe the straightforward fix here. I haven't looked at this
closely enough to see if this would work, though.
> But while trying to figure out the above, I noticed a few things that prolonged
> the time I needed to understand what was going on, so I would like to propose
> some more changes to make the code perhaps a bit easier to read for the next
> person who comes to hack on it after me.
>
> The description of the extra modifications is in the commit message. All of
> these changes are obviously optional and naturally subjective. I think that we
> can all agree on some points (less indentation = good), but naming is hard,
> and so is balance between unclear and too verbose, or when to split all
> non-functional changes to a separate commit. So let me know if there are things
> to do differently and I will gladly obey, it is your codebase after all.
Yes, please split non-functional changes into a separate commit
(preferably one for each concern). I do envision reviewers saying "let's
put patches X, Y, and Z in, but not patches A, B, and C", so splitting
would make it easier to decide what's worthwhile to have.
> Which brings me to the next topic, testing.
>
> Validating the fix would be trivial with a mocked libcurl, but turns out to be
> much harder with the integration-level test suite of this project.
[snip]
> But outside of that, and as far as the bundled test suite goes, I have failed to
> write a test that could validate this problem does not ever occur again.
Due to the nature of the bug and the fix, I do agree that it's hard to
test and I would be OK with including the fix without associated tests.
^ permalink raw reply
* Re: git-bisect reset not deleting .git/BISECT_LOG
From: Jeff King @ 2023-11-13 21:08 UTC (permalink / raw)
To: Janik Haag; +Cc: git
In-Reply-To: <bef9d5b3-bb64-4662-8952-d000872c5244@aq0.de>
On Mon, Nov 13, 2023 at 09:42:09PM +0100, Janik Haag wrote:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
> ```bash
> git init /tmp/reproduce-bisect-warning
> cd /tmp/reproduce-bisect-warning
> touch .git/BISECT_LOG
> git bisect reset
> git switch -c log
> ```
>
> What did you expect to happen? (Expected behavior)
>
> `git-bisect reset` should have deleted .git/BISECT_LOG
>
> What happened instead? (Actual behavior)
>
> .git/BISECT_LOG is still there
I don't think this is really specific to BISECT_LOG. In "bisect reset",
we'll call bisect_clean_state(), which removes a bunch of files. The
problem in your example is that "bisect reset" sees that we are not
bisecting and bails early.
Which I can kind of see, as part of the "reset" process is to reset to
the original pre-bisect commit. If it's not given on the command line,
the default is to use BISECT_START, which of course does not exist.
As a side note, "git bisect reset HEAD" will do what you want, because
it skips the BISECT_START check. But obviously that's not something
normal users should need to know about, and I think is even an
accidental side-effect of the conversion in 5e82c3dd22
(bisect--helper: `bisect_reset` shell function in C, 2019-01-02).
So really, you just want the "clean" part of "bisect reset", but not the
"reset". We could have a separate "bisect clean" that would do what you
want (clean state without trying to reset HEAD). But I don't think it
would be unreasonable to "reset" to just unconditionally clean. I think
it would probably just be a few lines in bisect_reset() to avoid the
early return, and skip the call to "checkout" when we have no branch to
go back to.
Maybe a good simple patch for somebody interested in getting into Git
development?
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox