* Re: [PATCH] revision: parse integer arguments to --max-count, --skip, etc., more carefully
From: Jeff King @ 2023-12-12 22:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Britton Kerin
In-Reply-To: <xmqqjzpjsbjl.fsf@gitster.g>
On Tue, Dec 12, 2023 at 07:09:02AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > This all looks pretty reasonable to me.
> >
> > I couldn't help but think, though, that surely we have some helpers for
> > this already? But the closest seems to be git_parse_int(), which also
> > allows unit factors. I'm not sure if allowing "-n 1k" would be a feature
> > or a bug. ;)
>
> The change in question is meant to be a pure fix to replace a careless
> use of atoi(). I do not mind to see a separate patch to add such a
> feature later on top.
Oh, I mostly meant that I would have turned to git_parse_int() as that
already-existing helper, but it is not suitable because of the extra
unit-handling. I think your patch draws the line in the right place.
> > I wonder if there are more spots that could benefit.
>
> "git grep -e 'atoi('" would give somebody motivated a decent set of
> #microproject ideas, but many hits are not suited for strtol_i(),
> which is designed to parse an integer at the end of a string. Some
> places use atoi() immediately followed by strspn() to skip over
> digits, which means they are parsing an integer and want to continue
> reading after the integer, which is incompatible with what
> strtol_i() wants to do. They need either a separate helper or an
> updated strtol_i() that optionally allows you to parse the prefix
> and report where the integer ended, e.g., something like:
Yeah, I agree this might be a good microproject (or leftoverbits) area,
and the semantics for the helper you propose make sense to me.
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/4] refs: propagate errno when reading special refs fails
From: Junio C Hamano @ 2023-12-12 20:28 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Phillip Wood
In-Reply-To: <24032a62e54fb37dad46c3ede7151efc8a7a8818.1702365291.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Some refs in Git are more special than others due to reasons explained
> in the next commit. These refs are read via `refs_read_special_head()`,
> but this function doesn't behave the same as when we try to read a
> normal ref. Most importantly, we do not propagate `failure_errno` in the
> case where the reference does not exist, which is behaviour that we rely
> on in many parts of Git.
>
> Fix this bug by propagating errno when `strbuf_read_file()` fails.
Hmph, I thought, judging from what [1/4] did, that your plan was to
use the refs API, instead of peeking directly into the filesystem,
to access these pseudo refs, and am a bit puzzled if it makes all
that much difference to fix errno handling while still reading
directly from the filesystem. Perhaps such a conversion happens in
later steps of this series (or a follow on series after this series
lands)?
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> refs.c | 4 +++-
> t/t1403-show-ref.sh | 10 ++++++++++
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index fcae5dddc6..00e72a2abf 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
> int result = -1;
> strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
>
> - if (strbuf_read_file(&content, full_path.buf, 0) < 0)
> + if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
> + *failure_errno = errno;
> goto done;
> + }
>
> result = parse_loose_ref_contents(content.buf, oid, referent, type,
> failure_errno);
> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> index b50ae6fcf1..66e6e77fa9 100755
> --- a/t/t1403-show-ref.sh
> +++ b/t/t1403-show-ref.sh
> @@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' '
> test_cmp expect err
> '
>
> +test_expect_success '--exists with non-existent special ref' '
> + test_expect_code 2 git show-ref --exists FETCH_HEAD
> +'
> +
> +test_expect_success '--exists with existing special ref' '
> + test_when_finished "rm .git/FETCH_HEAD" &&
> + git rev-parse HEAD >.git/FETCH_HEAD &&
> + git show-ref --exists FETCH_HEAD
> +'
> +
> test_done
^ permalink raw reply
* Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb
From: Junio C Hamano @ 2023-12-12 20:24 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Taylor Blau, Phillip Wood
In-Reply-To: <1db3eb3945432964aabe1c559db4c3ac251e83fd.1702365291.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The current code only works by chance because we only have a single
> reference backend implementation. Refactor it to instead read both refs
> via the refdb layer so that we'll also be compatible with alternate
> reference backends.
"via the refdb" -> "via the refs API" or something here and on the
title, and possibly elsewhere in the proposed log messages and
in-code comments in patches in this series, as I've never seen a
word "refdb" used in the context of this project.
I agree it is bad manners to be intimate with the implementation
details of the how files-backend stores HEAD and ORIG_HEAD.
> Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
> because we didn't resolve symrefs before either, and in practice none of
> the refs in "rebase-merge/" would be symbolic. We thus don't want to
> resolve symrefs with the new code either to retain the old behaviour.
Good to see a rewrite being careful like this.
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> wt-status.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 9f45bf6949..fe9e590b80 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
> static int split_commit_in_progress(struct wt_status *s)
> {
> int split_in_progress = 0;
> - char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> + struct object_id head_oid, orig_head_oid;
> + char *rebase_amend, *rebase_orig_head;
>
> if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> !s->branch || strcmp(s->branch, "HEAD"))
> return 0;
>
> - head = read_line_from_git_path("HEAD");
> - orig_head = read_line_from_git_path("ORIG_HEAD");
> + if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
> + read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
> + return 0;
> +
This made me wonder if we have changed behaviour when on an unborn
branch. In such a case, the original most likely would have read
"ref: blah" in "head" and compared it with "rebase_amend", which
would be a good way to ensure they would not match. I would not
know offhand what the updated code would do, but head_oid would be
uninitialized in such a case, so ...?
> rebase_amend = read_line_from_git_path("rebase-merge/amend");
> rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
>
> - if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> + if (!rebase_amend || !rebase_orig_head)
> ; /* fall through, no split in progress */
> else if (!strcmp(rebase_amend, rebase_orig_head))
> - split_in_progress = !!strcmp(head, rebase_amend);
> - else if (strcmp(orig_head, rebase_orig_head))
> + split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
> + else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
> split_in_progress = 1;
>
> - free(head);
> - free(orig_head);
> free(rebase_amend);
> free(rebase_orig_head);
^ permalink raw reply
* Re: [PATCH] Use ^=1 to toggle between 0 and 1
From: Jeff King @ 2023-12-12 20:09 UTC (permalink / raw)
To: AtariDreams via GitGitGadget; +Cc: git, AtariDreams, Seija Kijin
In-Reply-To: <pull.1620.git.git.1702401468082.gitgitgadget@gmail.com>
On Tue, Dec 12, 2023 at 05:17:47PM +0000, AtariDreams via GitGitGadget wrote:
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 70aff515acb..f9f2c9dd850 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -593,8 +593,8 @@ static void anonymize_ident_line(const char **beg, const char **end)
> struct ident_split split;
> const char *end_of_header;
>
> - out = &buffers[which_buffer++];
> - which_buffer %= ARRAY_SIZE(buffers);
> + out = &buffers[which_buffer];
> + which_buffer ^= 1;
In the current code, if the size of "buffers" is increased then
everything would just work. But your proposed code (rather subtly) makes
the assumption that ARRAY_SIZE(buffers) is 2.
So even leaving aside questions of readability, I think the existing
code is much more maintainable.
> diff --git a/diff.c b/diff.c
> index 2c602df10a3..91842b54753 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1191,7 +1191,7 @@ static void mark_color_as_moved(struct diff_options *o,
> &pmb_nr);
>
> if (contiguous && pmb_nr && moved_symbol == l->s)
> - flipped_block = (flipped_block + 1) % 2;
> + flipped_block ^= 1;
> else
> flipped_block = 0;
This one I do not see any problem with changing, though I think it is a
matter of opinion on which is more readable (I actually tend to think of
"x = 0 - x" as idiomatic for flipping).
> diff --git a/ident.c b/ident.c
> index cc7afdbf819..188826eed63 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -459,7 +459,7 @@ const char *fmt_ident(const char *name, const char *email,
> int want_name = !(flag & IDENT_NO_NAME);
>
> struct strbuf *ident = &ident_pool[index];
> - index = (index + 1) % ARRAY_SIZE(ident_pool);
> + index ^= 1;
>
> if (!email) {
> if (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)
This has the same problem as the first case.
> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
> index 70396fa3845..241136148a5 100644
> --- a/t/helper/test-path-utils.c
> +++ b/t/helper/test-path-utils.c
> @@ -185,7 +185,7 @@ static int check_dotfile(const char *x, const char **argv,
> int res = 0, expect = 1;
> for (; *argv; argv++) {
> if (!strcmp("--not", *argv))
> - expect = !expect;
> + expect ^= 1;
This one is not wrong, but IMHO it is more clear to express negation of
a boolean using "!" (i.e., what the code is already doing).
So of the four hunks, only the second one seems like a possible
improvement, and even there I am not sure the readability is better.
-Peff
^ permalink raw reply
* Re: Test breakage with zlib-ng
From: Jeff King @ 2023-12-12 20:01 UTC (permalink / raw)
To: René Scharfe
Cc: Ondrej Pohorelsky, git, brian m . carlson, Junio C Hamano
In-Reply-To: <9feeb6cf-aabf-4002-917f-3f6c27547bc8@web.de>
On Tue, Dec 12, 2023 at 06:04:55PM +0100, René Scharfe wrote:
> Subject: [PATCH] t6300: avoid hard-coding object sizes
>
> f4ee22b526 (ref-filter: add tests for objectsize:disk, 2018-12-24)
> hard-coded the expected object sizes. Coincidentally the size of commit
> and tag is the same with zlib at the default compression level.
>
> 1f5f8f3e85 (t6300: abstract away SHA-1-specific constants, 2020-02-22)
> encoded the sizes as a single value, which coincidentally also works
> with sha256.
>
> Different compression libraries like zlib-ng may arrive at different
> values. Get them from the file system instead of hard-coding them to
> make switching the compression library (or changing the compression
> level) easier.
Yeah, this is definitely the right solution here. I'm surprised the
hard-coded values didn't cause problems before now. ;)
The patch looks good to me, but a few small comments:
> +test_object_file_size () {
> + oid=$(git rev-parse "$1")
> + path=".git/objects/$(test_oid_to_path $oid)"
> + test_file_size "$path"
> +}
Here we're assuming the objects are loose. I think that's probably OK
(and certainly the test will notice if that changes).
We're covering the formatting code paths along with the underlying
implementation that fills in object_info->disk_sizep for loose objects.
Which I think is plenty for this particular script, which is about
for-each-ref.
It would be nice to have coverage of the packed_object_info() code path,
though. Back when it was added in a4ac106178 (cat-file: add
%(objectsize:disk) format atom, 2013-07-10), I cowardly punted on this,
writing:
This patch does not include any tests, as the exact numbers
returned are volatile and subject to zlib and packing
decisions. We cannot even reliably guarantee that the
on-disk size is smaller than the object content (though in
general this should be the case for non-trivial objects).
I don't think it's that big a deal, but I guess we could do something
like:
prev=
git show-index <$pack_idx |
sort -n |
grep -A1 $oid |
while read ofs oid csum
do
test -n "$prev" && echo "$((ofs - prev))"
prev=$ofs
done
It feels a little redundant with what Git is doing under the hood, but
at least is exercising the code (and we're using the idx directly, so
we're confirming that the revindex is right).
Anyway, that is all way beyond the scope of your patch, but I wonder if
it's worth doing on top.
> @@ -129,7 +129,7 @@ test_atom head push:strip=1 remotes/myfork/main
> test_atom head push:strip=-1 main
> test_atom head objecttype commit
> test_atom head objectsize $((131 + hexlen))
> -test_atom head objectsize:disk $disklen
> +test_atom head objectsize:disk $(test_object_file_size refs/heads/main)
These test_object_file_size calls are happening outside of any
test_expect_* block, so we'd miss failing exit codes (and also the
helper is not &&-chained), and any stderr would leak to the output.
That's probably OK in practice, though (if something goes wrong then the
expected value output will be bogus and the test itself will fail).
-Peff
^ permalink raw reply
* Re: [PATCH] tests: prefer host Git to verify chainlint self-checks
From: Junio C Hamano @ 2023-12-12 19:30 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Eric Sunshine
In-Reply-To: <4112adbe467c14a8f22a87ea41aa4705f8760cf6.1702380646.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> To accomodate for cases where the host system has no Git installation we
> use the locally-compiled version of Git. This can result in problems
> though when the Git project's repository is using extensions that the
> locally-compiled version of Git doesn't understand. It will refuse to
> run and thus cause the checks to fail.
>
> Fix this issue by prefering the host's Git resolved via PATH. If it
> doesn't exist, then we fall back to the locally-compiled Git version and
> diff as before.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> I've started to dogfood the reftable backend on my local machine and
> have converted many repositories to use the reftable backend. This
> surfaced the described issue because the repository now sets up the
> "extensions.refStorage" extension, and thus "check-chainlint" fails
> depending on which versions of Git I'm trying to compile and test.
I do not think "prefer host Git" is necessarily a good idea; falling
back to use host Git is perfectly fine, of course.
Other than that, I agree with the motivation.
> t/Makefile | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/Makefile b/t/Makefile
> index 225aaf78ed..8b7f7aceaa 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -111,7 +111,9 @@ check-chainlint:
> if test -f ../GIT-BUILD-OPTIONS; then \
> . ../GIT-BUILD-OPTIONS; \
> fi && \
> - if test -x ../git$$X; then \
> + if command -v git >/dev/null 2>&1; then \
> + DIFFW="git --no-pager diff -w --no-index"; \
> + elif test -x ../git$$X; then \
> DIFFW="../git$$X --no-pager diff -w --no-index"; \
> else \
> DIFFW="diff -w -u"; \
^ permalink raw reply
* Re: [PATCH v2 0/7] clone: fix init of refdb with wrong object format
From: Junio C Hamano @ 2023-12-12 19:20 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <cover.1702361370.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> this is the second version of my patch series to make git-clone(1)
> initialize the reference database with the correct hash format. This is
> a preparatory step for the reftable backend, which encodes the hash
> format on-disk and thus requires us to get the hash format right at the
> time of initialization.
>
> Changes compared to v1:
>
> - Patch 1: Extend the comment explaining why we always create the
> "refs/" directory.
>
> - Patch 3: Explain better why the patch is required in the first
> place.
>
> - Patch 4: Fix a typo.
>
> - Patch 7: Adapt a failing test to now assert the new behaviour.
>
> Thanks for your reviews!
Everything I see in the range-diff looks very sensible.
Will replace. Thanks.
^ permalink raw reply
* Re: Propose a change in open for passing in the file type.
From: Torsten Bögershausen @ 2023-12-12 17:29 UTC (permalink / raw)
To: Haritha D; +Cc: git@vger.kernel.org
In-Reply-To: <E1D54D98-3836-41CA-84B5-32AEAF7642D8@ibm.com>
On Tue, Dec 12, 2023 at 02:46:04PM +0000, Haritha D wrote:
> Hi Everyone,
>
> Am working on porting git to z/OS. For reference, the pull request am working on https://github.com/git/git/pull/1537.
>
> On z/OS there is a notion of file tag attributes. Files can be tagged as binary, ASCII, UTF8, EBCDIC, etc. z/OS uses these attributes to determine if auto-conversion is necessary. It was recommended in PR that we add logic directly to xopen . In order for me to do this in xopen , I have to pass an extra parameter to xopen that specifies the file type.
>
> Ex:
> xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
>
> To :
> xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666, BINARY);
>
> BINARY: would be an enum value.
>
> Would this be okay to do? Or are there other recommendations?
I think that the suggestion was to embedd the BINARY thing into xopen(),
not to all callers of xopen().
If you have this type of special handling for one specific OS,
call it quirks, if you want, they are better placed in an own file.
Please see e.g. compat/mingw.c as an example, how things are solved
for Git for windows.
Then there are some include files, which re-define things like open(),
if needed.
Another example could be
compat/win32/dirent.c
Where exactly things are placed, is may be a matter of taste,
that may be decided later.
In your case a file like
compat/z_os.c and z_os.h may make clear what this is about to everybody.
>
> Best regards
> Haritha
>
>
^ permalink raw reply
* Re: [PATCH] Use ^=1 to toggle between 0 and 1
From: Dragan Simic @ 2023-12-12 17:29 UTC (permalink / raw)
To: AtariDreams via GitGitGadget; +Cc: git, AtariDreams, Seija Kijin
In-Reply-To: <pull.1620.git.git.1702401468082.gitgitgadget@gmail.com>
On 2023-12-12 18:17, AtariDreams via GitGitGadget wrote:
> From: Seija Kijin <doremylover123@gmail.com>
>
> If it is known that an int is either 1 or 0,
> doing an exclusive or to switch instead of a
> modulus makes more sense and is more efficient.
Quite frankly, this doesn't seem like an improvement to me. It makes
the code much less readable, more error-prone, and may even end up
producing code that isn't portable.
Regarding the efficiency, such optimizations may be perfectly fine as a
trade-off in some critical paths, but these cases don't seem like that.
> Signed-off-by: Seija Kijin doremylover123@gmail.com
> ---
> Use ^=1 to toggle between 0 and 1
>
> If it is known that an int is either 1 or 0, doing an exclusive or
> to
> switch instead of a modulus makes more sense and is more efficient.
>
> Signed-off-by: Seija Kijin doremylover123@gmail.com
>
> Published-As:
> https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAtariDreams%2Fbuffer-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> pr-git-1620/AtariDreams/buffer-v1
> Pull-Request: https://github.com/git/git/pull/1620
>
> builtin/fast-export.c | 4 ++--
> diff.c | 2 +-
> ident.c | 2 +-
> t/helper/test-path-utils.c | 2 +-
> 4 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index 70aff515acb..f9f2c9dd850 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -593,8 +593,8 @@ static void anonymize_ident_line(const char **beg,
> const char **end)
> struct ident_split split;
> const char *end_of_header;
>
> - out = &buffers[which_buffer++];
> - which_buffer %= ARRAY_SIZE(buffers);
> + out = &buffers[which_buffer];
> + which_buffer ^= 1;
> strbuf_reset(out);
>
> /* skip "committer", "author", "tagger", etc */
> diff --git a/diff.c b/diff.c
> index 2c602df10a3..91842b54753 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1191,7 +1191,7 @@ static void mark_color_as_moved(struct
> diff_options *o,
> &pmb_nr);
>
> if (contiguous && pmb_nr && moved_symbol == l->s)
> - flipped_block = (flipped_block + 1) % 2;
> + flipped_block ^= 1;
> else
> flipped_block = 0;
>
> diff --git a/ident.c b/ident.c
> index cc7afdbf819..188826eed63 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -459,7 +459,7 @@ const char *fmt_ident(const char *name, const char
> *email,
> int want_name = !(flag & IDENT_NO_NAME);
>
> struct strbuf *ident = &ident_pool[index];
> - index = (index + 1) % ARRAY_SIZE(ident_pool);
> + index ^= 1;
>
> if (!email) {
> if (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)
> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
> index 70396fa3845..241136148a5 100644
> --- a/t/helper/test-path-utils.c
> +++ b/t/helper/test-path-utils.c
> @@ -185,7 +185,7 @@ static int check_dotfile(const char *x, const char
> **argv,
> int res = 0, expect = 1;
> for (; *argv; argv++) {
> if (!strcmp("--not", *argv))
> - expect = !expect;
> + expect ^= 1;
> else if (expect != (is_hfs(*argv) || is_ntfs(*argv)))
> res = error("'%s' is %s.git%s", *argv,
> expect ? "not " : "", x);
>
> base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
^ permalink raw reply
* [PATCH] Use ^=1 to toggle between 0 and 1
From: AtariDreams via GitGitGadget @ 2023-12-12 17:17 UTC (permalink / raw)
To: git; +Cc: AtariDreams, Seija Kijin
From: Seija Kijin <doremylover123@gmail.com>
If it is known that an int is either 1 or 0,
doing an exclusive or to switch instead of a
modulus makes more sense and is more efficient.
Signed-off-by: Seija Kijin doremylover123@gmail.com
---
Use ^=1 to toggle between 0 and 1
If it is known that an int is either 1 or 0, doing an exclusive or to
switch instead of a modulus makes more sense and is more efficient.
Signed-off-by: Seija Kijin doremylover123@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1620%2FAtariDreams%2Fbuffer-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1620/AtariDreams/buffer-v1
Pull-Request: https://github.com/git/git/pull/1620
builtin/fast-export.c | 4 ++--
diff.c | 2 +-
ident.c | 2 +-
t/helper/test-path-utils.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 70aff515acb..f9f2c9dd850 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -593,8 +593,8 @@ static void anonymize_ident_line(const char **beg, const char **end)
struct ident_split split;
const char *end_of_header;
- out = &buffers[which_buffer++];
- which_buffer %= ARRAY_SIZE(buffers);
+ out = &buffers[which_buffer];
+ which_buffer ^= 1;
strbuf_reset(out);
/* skip "committer", "author", "tagger", etc */
diff --git a/diff.c b/diff.c
index 2c602df10a3..91842b54753 100644
--- a/diff.c
+++ b/diff.c
@@ -1191,7 +1191,7 @@ static void mark_color_as_moved(struct diff_options *o,
&pmb_nr);
if (contiguous && pmb_nr && moved_symbol == l->s)
- flipped_block = (flipped_block + 1) % 2;
+ flipped_block ^= 1;
else
flipped_block = 0;
diff --git a/ident.c b/ident.c
index cc7afdbf819..188826eed63 100644
--- a/ident.c
+++ b/ident.c
@@ -459,7 +459,7 @@ const char *fmt_ident(const char *name, const char *email,
int want_name = !(flag & IDENT_NO_NAME);
struct strbuf *ident = &ident_pool[index];
- index = (index + 1) % ARRAY_SIZE(ident_pool);
+ index ^= 1;
if (!email) {
if (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len)
diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
index 70396fa3845..241136148a5 100644
--- a/t/helper/test-path-utils.c
+++ b/t/helper/test-path-utils.c
@@ -185,7 +185,7 @@ static int check_dotfile(const char *x, const char **argv,
int res = 0, expect = 1;
for (; *argv; argv++) {
if (!strcmp("--not", *argv))
- expect = !expect;
+ expect ^= 1;
else if (expect != (is_hfs(*argv) || is_ntfs(*argv)))
res = error("'%s' is %s.git%s", *argv,
expect ? "not " : "", x);
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: Test breakage with zlib-ng
From: René Scharfe @ 2023-12-12 17:04 UTC (permalink / raw)
To: Ondrej Pohorelsky, git; +Cc: brian m . carlson, Junio C Hamano
In-Reply-To: <CA+B51BEpSh1wT627Efpysw3evVocpiDCoQ3Xaza6jKE3B62yig@mail.gmail.com>
Am 12.12.23 um 15:16 schrieb Ondrej Pohorelsky:
> Hi everyone,
>
> As some might have heard, there is a proposal for Fedora 40 to
> transition from zlib to zlib-ng[0]. Because of this, there has been a
> rebuild of all packages to ensure every package works under zlib-ng.
>
> Git test suit has three breakages in t6300-for-each-ref.sh.
> To be precise, it is:
>
> not ok 35 - basic atom: refs/heads/main objectsize:disk
> not ok 107 - basic atom: refs/tags/testtag objectsize:disk
> not ok 108 - basic atom: refs/tags/testtag *objectsize:disk
>
>
> All of these tests are atomic, and they compare the result against
> $disklen.
Why do these three objects (HEAD commit of main, testtag and testtag
target) have the same size? Half of the answer is that testtag points
to the HEAD of main. But the other half is pure coincidence as far as I
can see.
> I can easily patch these tests in Fedora to be compatible with zlib-ng
> only by not comparing to $disklen, but a concrete value, however I
> would like to have a universal solution that works with both zlib and
> zlib-ng. So if anyone has an idea on how to do it, please let me know.
The test stores the expected values at the top, in the following lines,
for the two possible repository formats:
test_expect_success setup '
test_oid_cache <<-EOF &&
disklen sha1:138
disklen sha256:154
EOF
So it's using hard-coded values already, which breaks when the
compression rate changes.
We could set core.compression to 0 to take compression out of the
picture.
Or we could get the sizes of the objects by checking their files,
which would not require hard-coding anymore. Patch below.
--- >8 ---
Subject: [PATCH] t6300: avoid hard-coding object sizes
f4ee22b526 (ref-filter: add tests for objectsize:disk, 2018-12-24)
hard-coded the expected object sizes. Coincidentally the size of commit
and tag is the same with zlib at the default compression level.
1f5f8f3e85 (t6300: abstract away SHA-1-specific constants, 2020-02-22)
encoded the sizes as a single value, which coincidentally also works
with sha256.
Different compression libraries like zlib-ng may arrive at different
values. Get them from the file system instead of hard-coding them to
make switching the compression library (or changing the compression
level) easier.
Reported-by: Ondrej Pohorelsky <opohorel@redhat.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
t/t6300-for-each-ref.sh | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 54e2281259..843a7fe143 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -20,12 +20,13 @@ setdate_and_increment () {
export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
}
-test_expect_success setup '
- test_oid_cache <<-EOF &&
- disklen sha1:138
- disklen sha256:154
- EOF
+test_object_file_size () {
+ oid=$(git rev-parse "$1")
+ path=".git/objects/$(test_oid_to_path $oid)"
+ test_file_size "$path"
+}
+test_expect_success setup '
# setup .mailmap
cat >.mailmap <<-EOF &&
A Thor <athor@example.com> A U Thor <author@example.com>
@@ -94,7 +95,6 @@ test_atom () {
}
hexlen=$(test_oid hexsz)
-disklen=$(test_oid disklen)
test_atom head refname refs/heads/main
test_atom head refname: refs/heads/main
@@ -129,7 +129,7 @@ test_atom head push:strip=1 remotes/myfork/main
test_atom head push:strip=-1 main
test_atom head objecttype commit
test_atom head objectsize $((131 + hexlen))
-test_atom head objectsize:disk $disklen
+test_atom head objectsize:disk $(test_object_file_size refs/heads/main)
test_atom head deltabase $ZERO_OID
test_atom head objectname $(git rev-parse refs/heads/main)
test_atom head objectname:short $(git rev-parse --short refs/heads/main)
@@ -203,8 +203,8 @@ test_atom tag upstream ''
test_atom tag push ''
test_atom tag objecttype tag
test_atom tag objectsize $((114 + hexlen))
-test_atom tag objectsize:disk $disklen
-test_atom tag '*objectsize:disk' $disklen
+test_atom tag objectsize:disk $(test_object_file_size refs/tags/testtag)
+test_atom tag '*objectsize:disk' $(test_object_file_size refs/heads/main)
test_atom tag deltabase $ZERO_OID
test_atom tag '*deltabase' $ZERO_OID
test_atom tag objectname $(git rev-parse refs/tags/testtag)
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 02/24] pack-bitmap-write: deep-clear the `bb_commit` slab
From: Taylor Blau @ 2023-12-12 16:48 UTC (permalink / raw)
To: Jeff King; +Cc: git, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <20231212070406.GA1117953@coredump.intra.peff.net>
On Tue, Dec 12, 2023 at 02:04:06AM -0500, Jeff King wrote:
> On Tue, Nov 28, 2023 at 02:07:59PM -0500, Taylor Blau wrote:
>
> > +static void clear_bb_commit(struct bb_commit *commit)
> > +{
> > + free(commit->reverse_edges);
> > + bitmap_free(commit->commit_mask);
> > + bitmap_free(commit->bitmap);
> > +}
>
> I think these bitmaps may sometimes be NULL. But double-checking
> bitmap_free(), it sensibly is noop when passed NULL. So this look good
> to me.
Yeah, bitamp_free() handles a NULL input correctly, so we can pass a
possibly-NULL `commit->commit_mask` or `commit->bitmap` argument and be
OK.
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH v6] subtree: fix split processing with multiple subtrees present
From: Christian Couder @ 2023-12-12 16:06 UTC (permalink / raw)
To: Zach FettersMoore; +Cc: Zach FettersMoore via GitGitGadget, git
In-Reply-To: <CAEWN6q3RTbVuMb0VyCYz196ZL+OGAAHbJLZ2-MnW1RVVabg7Mw@mail.gmail.com>
On Mon, Dec 11, 2023 at 4:39 PM Zach FettersMoore
<zach.fetters@apollographql.com> wrote:
>
> >>
> >> From: Zach FettersMoore <zach.fetters@apollographql.com>
> >> To see this in practice you can use the open source GitHub repo
> >> 'apollo-ios-dev' and do the following in order:
> >>
> >> -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'
> >> directories
> >> -Create a commit containing these changes
> >> -Do a split on apollo-ios-codegen
> >> - Do a fetch on the subtree repo
> >> - git fetch git@github.com:apollographql/apollo-ios-codegen.git
> >> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin
>
> > Now I get the following without your patch at this step:
> >
> > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin
> > [...]/libexec/git-core/git-subtree: 318: Maximum function recursion
> > depth (1000) reached
> >
> > Line 318 in git-subtree.sh contains the following:
> >
> > missed=$(cache_miss "$@") || exit $?
> >
> > With your patch it seems to work:
> >
> > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin
> > Merge made by the 'ort' strategy.
> > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4
>
> Looking into this some it looks like it could be a bash config
> difference? My machine always runs it all the way through vs
> failing for recursion depth. Although that would also be an issue
> which is solved by this fix.
I use Ubuntu where /bin/sh is dash so my current guess is that dash
might have a smaller recursion limit than bash.
I just found https://stackoverflow.com/questions/69493528/git-subtree-maximum-function-recursion-depth
which seems to agree.
I will try to test using bash soon.
> >> - Depending on the current state of the 'apollo-ios-dev' repo
> >> you may see the issue at this point if the last split was on
> >> apollo-ios
>
> > I guess I see it, but it seems a bit different for me than what you describe.
> >
> > Otherwise your patch looks good to me now.
>
> Yea I hadn't accounted for/realized that some folks may see a recursion
> depth error vs it just taking a long time like it does for me. Also what
> I was saying with the apollo-ios-dev repo is you may not need all the steps
> to see the issue, because its possible the state of the repo is already
> in a position to display the issue just by doing a split on
> apollo-ios-codegen.
>
> Great! Thanks again for all the feedback and guidance! Is there anything
> else I need to do to get this across the finish line and merged in?
Hopefully I will be able to confirm I see the same error as you with
bash soon, and it will be enough to get it merged.
^ permalink raw reply
* Feature Request: Add confirm message when executed prune
From: Miguel de Dios Matias @ 2023-12-12 15:18 UTC (permalink / raw)
To: git
Hi.
I accidentallly did a git push --prune --all in a git from the company
where I am working (and hope I will be working...well sorry for the
joke), because it is simple I mistook git pull with git push and I
deleted all branches of my jobmates.
I thing it might be a good idea to add by default a confirmation
message (can disabled by conf).
Or add a parameter such as rm --interactive for to avoid or minimice
(depends of coffe level) these kind mistakes.
Regards.
^ permalink raw reply
* Re: [PATCH] revision: parse integer arguments to --max-count, --skip, etc., more carefully
From: Junio C Hamano @ 2023-12-12 15:09 UTC (permalink / raw)
To: Jeff King; +Cc: git, Britton Kerin
In-Reply-To: <20231212013045.GE376323@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This all looks pretty reasonable to me.
>
> I couldn't help but think, though, that surely we have some helpers for
> this already? But the closest seems to be git_parse_int(), which also
> allows unit factors. I'm not sure if allowing "-n 1k" would be a feature
> or a bug. ;)
The change in question is meant to be a pure fix to replace a careless
use of atoi(). I do not mind to see a separate patch to add such a
feature later on top.
> I guess "strtol_i()" maybe is that helper already, though I did not even
> know it existed. Looks like it goes back to 2007, and is seldom used.
I didn't know about it, either ;-) I used it only because there is
one use of it, among places that used atoi() I wanted to tighten.
> I wonder if there are more spots that could benefit.
"git grep -e 'atoi('" would give somebody motivated a decent set of
#microproject ideas, but many hits are not suited for strtol_i(),
which is designed to parse an integer at the end of a string. Some
places use atoi() immediately followed by strspn() to skip over
digits, which means they are parsing an integer and want to continue
reading after the integer, which is incompatible with what
strtol_i() wants to do. They need either a separate helper or an
updated strtol_i() that optionally allows you to parse the prefix
and report where the integer ended, e.g., something like:
#define strtol_i(s,b,r) strtol_i2((s), (b), (r), NULL)
static inline int strtol_i2(char const *s, int base, int *result, char **endp)
{
long ul;
char *dummy = NULL;
if (!endp)
endp = &dummy;
errno = 0;
ul = strtol(s, &endp, base);
if (errno ||
/*
* if we are told to parse to the end of the string by
* passing NULL to endp, it is an error to have any
* remaining character after the digits.
*/
(dummy && *dummy) ||
endp == s || (int) ul != ul)
return -1;
*result = ul;
return 0;
}
perhaps.
^ permalink raw reply
* Propose a change in open for passing in the file type.
From: Haritha D @ 2023-12-12 14:46 UTC (permalink / raw)
To: git@vger.kernel.org
Hi Everyone,
Am working on porting git to z/OS. For reference, the pull request am working on https://github.com/git/git/pull/1537.
On z/OS there is a notion of file tag attributes. Files can be tagged as binary, ASCII, UTF8, EBCDIC, etc. z/OS uses these attributes to determine if auto-conversion is necessary. It was recommended in PR that we add logic directly to xopen . In order for me to do this in xopen , I have to pass an extra parameter to xopen that specifies the file type.
Ex:
xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
To :
xopen(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666, BINARY);
BINARY: would be an enum value.
Would this be okay to do? Or are there other recommendations?
Best regards
Haritha
^ permalink raw reply
* Test breakage with zlib-ng
From: Ondrej Pohorelsky @ 2023-12-12 14:16 UTC (permalink / raw)
To: git
Hi everyone,
As some might have heard, there is a proposal for Fedora 40 to
transition from zlib to zlib-ng[0]. Because of this, there has been a
rebuild of all packages to ensure every package works under zlib-ng.
Git test suit has three breakages in t6300-for-each-ref.sh.
To be precise, it is:
not ok 35 - basic atom: refs/heads/main objectsize:disk
not ok 107 - basic atom: refs/tags/testtag objectsize:disk
not ok 108 - basic atom: refs/tags/testtag *objectsize:disk
All of these tests are atomic, and they compare the result against
$disklen. I discussed it with Tulio Magno Quites Machado Filho, who
ran the tests and is owner of the proposal.
It seems like the compression of zlib-ng is shaving/adding some bytes
to the actual output, which then fails the comparison.
Here is an example:
```
expecting success of 6300.35 'basic atom: refs/heads/main objectsize:disk':
git for-each-ref --format="%($format)" "$ref" >actual &&
sanitize_pgp <actual >actual.clean &&
test_cmp expected actual.clean
++ git for-each-ref '--format=%(objectsize:disk)' refs/heads/main
++ sanitize_pgp
++ perl -ne '
/^-----END PGP/ and $in_pgp = 0;
print unless $in_pgp;
/^-----BEGIN PGP/ and $in_pgp = 1;
'
++ command /usr/bin/perl -ne '
/^-----END PGP/ and $in_pgp = 0;
print unless $in_pgp;
/^-----BEGIN PGP/ and $in_pgp = 1;
'
++ test_cmp expected actual.clean
++ test 2 -ne 2
++ eval 'diff -u' '"$@"'
+++ diff -u expected actual.clean
--- expected 2023-12-06 21:06:07.808849497 +0000
+++ actual.clean 2023-12-06 21:06:07.812849541 +0000
@@ -1 +1 @@
-138
+137
error: last command exited with $?=1
not ok 35 - basic atom: refs/heads/main objectsize:disk
```
The whole build log can be found here[1].
I can easily patch these tests in Fedora to be compatible with zlib-ng
only by not comparing to $disklen, but a concrete value, however I
would like to have a universal solution that works with both zlib and
zlib-ng. So if anyone has an idea on how to do it, please let me know.
Thanks
[0]https://discussion.fedoraproject.org/t/f40-change-proposal-transitioning-to-zlib-ng-as-a-compatible-replacement-for-zlib-system-wide/95807
[1]https://download.copr.fedorainfracloud.org/results/tuliom/zlib-ng-compat-mpb/fedora-rawhide-x86_64/06729801-git/builder-live.log.gz
Cheers,
Ondřej Pohořelský
^ permalink raw reply
* [PATCH] tests: prefer host Git to verify chainlint self-checks
From: Patrick Steinhardt @ 2023-12-12 11:32 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]
The "check-chainlint" target runs automatically when running tests and
performs self-checks to verify that the chainlinter itself produces the
expected output. Originally, the chainlinter was implemented via sed,
but the infrastructure has been rewritten in fb41727b7e (t: retire
unused chainlint.sed, 2022-09-01) to use a Perl script instead.
The rewrite caused some slight whitespace changes in the output that are
ultimately not of much importance. In order to be able to assert that
the actual chainlinter errors match our expectations we thus have to
ignore whitespace characters when diffing them. As the `-w` flag is not
in POSIX we try to use `git diff -w --no-index` before we fall back to
`diff -w -u`.
To accomodate for cases where the host system has no Git installation we
use the locally-compiled version of Git. This can result in problems
though when the Git project's repository is using extensions that the
locally-compiled version of Git doesn't understand. It will refuse to
run and thus cause the checks to fail.
Fix this issue by prefering the host's Git resolved via PATH. If it
doesn't exist, then we fall back to the locally-compiled Git version and
diff as before.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
I've started to dogfood the reftable backend on my local machine and
have converted many repositories to use the reftable backend. This
surfaced the described issue because the repository now sets up the
"extensions.refStorage" extension, and thus "check-chainlint" fails
depending on which versions of Git I'm trying to compile and test.
t/Makefile | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/t/Makefile b/t/Makefile
index 225aaf78ed..8b7f7aceaa 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -111,7 +111,9 @@ check-chainlint:
if test -f ../GIT-BUILD-OPTIONS; then \
. ../GIT-BUILD-OPTIONS; \
fi && \
- if test -x ../git$$X; then \
+ if command -v git >/dev/null 2>&1; then \
+ DIFFW="git --no-pager diff -w --no-index"; \
+ elif test -x ../git$$X; then \
DIFFW="../git$$X --no-pager diff -w --no-index"; \
else \
DIFFW="diff -w -u"; \
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH 00/24] pack-objects: multi-pack verbatim reuse
From: Jeff King @ 2023-12-12 8:12 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Patrick Steinhardt, Junio C Hamano
In-Reply-To: <cover.1701198172.git.me@ttaylorr.com>
On Tue, Nov 28, 2023 at 02:07:54PM -0500, Taylor Blau wrote:
> Performing verbatim pack reuse naturally trades off between CPU time and
> the resulting pack size. In the above example, the single-pack reuse
> case produces a clone size of ~194 MB on my machine, while the
> multi-pack reuse case produces a clone size closer to ~266 MB, which is
> a ~37% increase in clone size.
Right, it's definitely a tradeoff. So taking a really big step back,
there are a few optimizations all tied up in the verbatim reuse code:
1. in some cases we get to dump whole swaths of the on-disk packfile
to the output, covering many objects with a few memcpy() calls.
(This is still O(n), of course, but it's fewer instructions per
object).
2. any other reused objects have only a small-ish amount of work to
fix up ofs deltas, handle gaps, and so on. We get to skip adding
them to the packing_list struct (this saves some CPU, but also a
lot of memory)
3. we skip the delta search for these reused objects. This is where
your big CPU / output size tradeoff comes into play, I'd think.
So my question is: how much of what you're seeing is from (1) and (2),
and how much is from (3)? Because there are other ways to trigger (3),
such as lowering the window size. For example, if you try your same
packing example with --window=0, how do the CPU and output size compare
to the results of your series? (I'd also check peak memory usage).
-Peff
^ permalink raw reply
* Re: [PATCH 05/24] midx: implement `DISP` chunk
From: Jeff King @ 2023-12-12 8:03 UTC (permalink / raw)
To: Taylor Blau; +Cc: Junio C Hamano, git, Patrick Steinhardt
In-Reply-To: <ZXPRL5yCTQKr0k7e@nand.local>
On Fri, Dec 08, 2023 at 09:30:07PM -0500, Taylor Blau wrote:
> > I did wonder how expensive to recompute and validate the "distinct"
> > information (in other words, is it too expensive for the consumers
> > of an existing midx file to see which packs are distinct on demand
> > before they stream contents out of the underlying packs?), as the
> > way the packs are marked as distinct looked rather error prone (you
> > can very easily list packfiles with overlaps with "+" prefix and the
> > DISK chunk writer does not even notice that you lied to it). As long
> > as "git fsck" catches when two packs that are marked as distinct share
> > an object, that is OK, but the arrangement did look rather brittle
> > to me.
>
> It's likely too expensive to do on the reading side for every
> pack-objects operation or MIDX load.
This made me think a bit. Obviously we can check for disjointedness in
O(n log k), where n is the total number of objects and k is the number
of packs, by doing a k-merge of the sorted lists. But that's a
non-starter, because we may be serving a request that is much smaller
than all "n" objects (e.g., any small fetch, but also even clones when
the pack contains a bunch of irrelevant objects).
But we can relax our condition a bit. The packs only need to be disjoint
with respect to the set of objects that we are going to output (we'll
call that "m"). So at the very least, you can do O(mk) lookups (each one
itself "log n", of course). We know that the work is already going to
be linear in "m". In theory we want to generally keep "k" small, but
part of the point of using midx in this way is to let "k" grow a bit.
So that might turn out pretty bad in practice.
So let's take one more step back. One thing I didn't feel like I saw
either in this patch or the cover letter is exactly why we care about
disjointedness. IIRC, the main issue is making sure that for any object
X we send verbatim, it is either a non-delta or its delta base is
viable. And the two reasons I can think of for the base to be non-viable
are:
1. We are not sending the base at all.
2. The base is in another pack, and we are worried about creating a
cycle (i.e., in pack A we have X as a delta against Y, and in pack
B we have Y as a delta against X, and we send both deltas).
We already deal with (1) for the single-pack case by finding the base
object offset, converting it to a pack position, and then making sure
that position is also marked for verbatim reuse.
The interesting one is (2), which is new for midx (since a single pack
cannot have two copies of an object). But I'm not sure if it's possible.
The verbatim reuse code depends on using bitmaps in the first place. And
there is already a preference-order to the packs in the midx bitmaps.
That is, we'll choose all of the objects for pack A over any duplicates
in B, and duplicates from B over C, and so on. If we likewise try
verbatim reuse in that order, then we know that an object in pack A can
never have a base that is selected from pack B or C (because if they do
have duplicates, we'd have selected A's copy to put in the midx bitmap).
And likewise, a copy of an object in pack B will always have its base
either in A or B, but never in C.
So it kind of seems to me that this would "just work" if
try_partial_reuse() did something like for the midx case:
- convert offset of base object into an object id hash using the pack
revindex (similar to offset_to_pack_pos)
- look up the object id in the midx to get a pack/offset combo
- use the midx revindex to convert that into a bit position
- check if that bit is marked for verbatim reuse
The assumption there is that in the second step, the midx has resolved
duplicates by putting all of pack A before pack B, and so on, as above.
It also assumes that we are trying verbatim reuse in the same order
(though a different order would not produce wrong results, it would only
result in less verbatim reuse).
All of which makes me think I'm missing some other case that is a
problem. While I wait for you to explain it, though, let's continue our
thought experiment for a moment.
If we assume that any cross-pack deltas are a problem, we could always
just skip them for verbatim reuse. That is, once we look up the object
id in the midx, we can see if it's in the same pack we're currently
processing. If not, we could punt and let the non-verbatim code paths
handle it as usual.
That still leaves the problem of realizing we skipped over a chunk of
the packfile (say we are pulling object X from pack B, and it is a delta
of Y, but we already decided to reuse Y from pack A). But the reuse code
already has to accommodate gaps. I think it happens naturally in
write_reused_pack_one(), where we feed the actual byte offsets into
record_reused_object(). You'd have to take some care not to go past gaps
in the blind copy that happens write_reused_pack_verbatim(). So you
might need to mark the first such gap somehow (if it's hard, I'd suggest
just skipping write_reused_pack_verbatim() entirely; it is a fairly
minor optimization compared to the rest of it).
And of course there's a bunch of hard work teaching all of those
functions about midx's and multiple packs in the first place, but you
already had to do all that in the latter part of your series, and it
would still be applicable.
OK, this is the part where you tell me what I'm missing. ;)
-Peff
^ permalink raw reply
* [PATCH v2 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb
From: Patrick Steinhardt @ 2023-12-12 7:19 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Phillip Wood
In-Reply-To: <cover.1702365291.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5649 bytes --]
We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem
and via the refdb, which violates the newly established rules for how
special refs must be treated. This works alright in practice with the
reffiles reference backend, but will cause bugs once we gain additional
backends.
Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb
so that it is no longer a special ref.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
bisect.c | 25 ++++---------------------
builtin/bisect.c | 8 ++------
refs.c | 3 ++-
t/t6030-bisect-porcelain.sh | 2 +-
4 files changed, 9 insertions(+), 29 deletions(-)
diff --git a/bisect.c b/bisect.c
index 1be8e0a271..985b96ed13 100644
--- a/bisect.c
+++ b/bisect.c
@@ -471,7 +471,6 @@ static int read_bisect_refs(void)
}
static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
@@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried,
static int is_expected_rev(const struct object_id *oid)
{
- const char *filename = git_path_bisect_expected_rev();
- struct stat st;
- struct strbuf str = STRBUF_INIT;
- FILE *fp;
- int res = 0;
-
- if (stat(filename, &st) || !S_ISREG(st.st_mode))
+ struct object_id expected_oid;
+ if (read_ref("BISECT_EXPECTED_REV", &expected_oid))
return 0;
-
- fp = fopen_or_warn(filename, "r");
- if (!fp)
- return 0;
-
- if (strbuf_getline_lf(&str, fp) != EOF)
- res = !strcmp(str.buf, oid_to_hex(oid));
-
- strbuf_release(&str);
- fclose(fp);
-
- return res;
+ return oideq(oid, &expected_oid);
}
enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
@@ -1185,10 +1168,10 @@ int bisect_clean_state(void)
struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
+ string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
result = delete_refs("bisect: remove", &refs_for_removal, REF_NO_DEREF);
refs_for_removal.strdup_strings = 1;
string_list_clear(&refs_for_removal, 0);
- unlink_or_warn(git_path_bisect_expected_rev());
unlink_or_warn(git_path_bisect_ancestors_ok());
unlink_or_warn(git_path_bisect_log());
unlink_or_warn(git_path_bisect_names());
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 35938b05fd..4e2c43daf5 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -17,7 +17,6 @@
#include "revision.h"
static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
@@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
const char *state;
int i, verify_expected = 1;
struct object_id oid, expected;
- struct strbuf buf = STRBUF_INIT;
struct oid_array revs = OID_ARRAY_INIT;
if (!argc)
@@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
oid_array_append(&revs, &commit->object.oid);
}
- if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
- get_oid_hex(buf.buf, &expected) < 0)
+ if (read_ref("BISECT_EXPECTED_REV", &expected))
verify_expected = 0; /* Ignore invalid file contents */
- strbuf_release(&buf);
for (i = 0; i < revs.nr; i++) {
if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) {
@@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
}
if (verify_expected && !oideq(&revs.oid[i], &expected)) {
unlink_or_warn(git_path_bisect_ancestors_ok());
- unlink_or_warn(git_path_bisect_expected_rev());
+ delete_ref(NULL, "BISECT_EXPECTED_REV", NULL, REF_NO_DEREF);
verify_expected = 0;
}
}
diff --git a/refs.c b/refs.c
index 8fe34d51e4..c76ce86bef 100644
--- a/refs.c
+++ b/refs.c
@@ -1840,6 +1840,8 @@ static int is_special_ref(const char *refname)
* There are some exceptions that you might expect to see on this list
* but which are handled exclusively via the reference backend:
*
+ * - BISECT_EXPECTED_REV
+ *
* - CHERRY_PICK_HEAD
*
* - HEAD
@@ -1857,7 +1859,6 @@ static int is_special_ref(const char *refname)
*/
static const char * const special_refs[] = {
"AUTO_MERGE",
- "BISECT_EXPECTED_REV",
"FETCH_HEAD",
"MERGE_AUTOSTASH",
"MERGE_HEAD",
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 2a5b7d8379..792c1504bc 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
git bisect bad $HASH4 &&
git bisect reset &&
test -z "$(git for-each-ref "refs/bisect/*")" &&
- test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
+ test_ref_missing BISECT_EXPECTED_REV &&
test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
test_path_is_missing ".git/BISECT_LOG" &&
test_path_is_missing ".git/BISECT_RUN" &&
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 3/4] refs: complete list of special refs
From: Patrick Steinhardt @ 2023-12-12 7:18 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Phillip Wood
In-Reply-To: <cover.1702365291.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4042 bytes --]
We have some references that are more special than others. The reason
for them being special is that they either do not follow the usual
format of references, or that they are written to the filesystem
directly by the respective owning subsystem and thus circumvent the
reference backend.
This works perfectly fine right now because the reffiles backend will
know how to read those refs just fine. But with the prospect of gaining
a new reference backend implementation we need to be a lot more careful
here:
- We need to make sure that we are consistent about how those refs are
written. They must either always be written via the filesystem, or
they must always be written via the reference backend. Any mixture
will lead to inconsistent state.
- We need to make sure that such special refs are always handled
specially when reading them.
We're already mostly good with regard to the first item, except for
`BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
But the current list of special refs is missing a lot of refs that
really should be treated specially. Right now, we only treat
`FETCH_HEAD` and `MERGE_HEAD` specially here.
Introduce a new function `is_special_ref()` that contains all current
instances of special refs to fix the reading path.
Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 00e72a2abf..8fe34d51e4 100644
--- a/refs.c
+++ b/refs.c
@@ -1820,15 +1820,65 @@ static int refs_read_special_head(struct ref_store *ref_store,
return result;
}
+static int is_special_ref(const char *refname)
+{
+ /*
+ * Special references get written and read directly via the filesystem
+ * by the subsystems that create them. Thus, they must not go through
+ * the reference backend but must instead be read directly. It is
+ * arguable whether this behaviour is sensible, or whether it's simply
+ * a leaky abstraction enabled by us only having a single reference
+ * backend implementation. But at least for a subset of references it
+ * indeed does make sense to treat them specially:
+ *
+ * - FETCH_HEAD may contain multiple object IDs, and each one of them
+ * carries additional metadata like where it came from.
+ *
+ * - MERGE_HEAD may contain multiple object IDs when merging multiple
+ * heads.
+ *
+ * There are some exceptions that you might expect to see on this list
+ * but which are handled exclusively via the reference backend:
+ *
+ * - CHERRY_PICK_HEAD
+ *
+ * - HEAD
+ *
+ * - ORIG_HEAD
+ *
+ * - "rebase-apply/" and "rebase-merge/" contain all of the state for
+ * rebases, including some reference-like files. These are
+ * exclusively read and written via the filesystem and never go
+ * through the refdb.
+ *
+ * Writing or deleting references must consistently go either through
+ * the filesystem (special refs) or through the reference backend
+ * (normal ones).
+ */
+ static const char * const special_refs[] = {
+ "AUTO_MERGE",
+ "BISECT_EXPECTED_REV",
+ "FETCH_HEAD",
+ "MERGE_AUTOSTASH",
+ "MERGE_HEAD",
+ };
+ size_t i;
+
+ for (i = 0; i < ARRAY_SIZE(special_refs); i++)
+ if (!strcmp(refname, special_refs[i]))
+ return 1;
+
+ return 0;
+}
+
int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
struct object_id *oid, struct strbuf *referent,
unsigned int *type, int *failure_errno)
{
assert(failure_errno);
- if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
+ if (is_special_ref(refname))
return refs_read_special_head(ref_store, refname, oid, referent,
type, failure_errno);
- }
return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
type, failure_errno);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 2/4] refs: propagate errno when reading special refs fails
From: Patrick Steinhardt @ 2023-12-12 7:18 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Phillip Wood
In-Reply-To: <cover.1702365291.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]
Some refs in Git are more special than others due to reasons explained
in the next commit. These refs are read via `refs_read_special_head()`,
but this function doesn't behave the same as when we try to read a
normal ref. Most importantly, we do not propagate `failure_errno` in the
case where the reference does not exist, which is behaviour that we rely
on in many parts of Git.
Fix this bug by propagating errno when `strbuf_read_file()` fails.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 4 +++-
t/t1403-show-ref.sh | 10 ++++++++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index fcae5dddc6..00e72a2abf 100644
--- a/refs.c
+++ b/refs.c
@@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
int result = -1;
strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
- if (strbuf_read_file(&content, full_path.buf, 0) < 0)
+ if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
+ *failure_errno = errno;
goto done;
+ }
result = parse_loose_ref_contents(content.buf, oid, referent, type,
failure_errno);
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index b50ae6fcf1..66e6e77fa9 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' '
test_cmp expect err
'
+test_expect_success '--exists with non-existent special ref' '
+ test_expect_code 2 git show-ref --exists FETCH_HEAD
+'
+
+test_expect_success '--exists with existing special ref' '
+ test_when_finished "rm .git/FETCH_HEAD" &&
+ git rev-parse HEAD >.git/FETCH_HEAD &&
+ git show-ref --exists FETCH_HEAD
+'
+
test_done
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb
From: Patrick Steinhardt @ 2023-12-12 7:18 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Phillip Wood
In-Reply-To: <cover.1702365291.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]
We read both the HEAD and ORIG_HEAD references directly from the
filesystem in order to figure out whether we're currently splitting a
commit. If both of the following are true:
- HEAD points to the same object as "rebase-merge/amend".
- ORIG_HEAD points to the same object as "rebase-merge/orig-head".
Then we are currently splitting commits.
The current code only works by chance because we only have a single
reference backend implementation. Refactor it to instead read both refs
via the refdb layer so that we'll also be compatible with alternate
reference backends.
Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
because we didn't resolve symrefs before either, and in practice none of
the refs in "rebase-merge/" would be symbolic. We thus don't want to
resolve symrefs with the new code either to retain the old behaviour.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
wt-status.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 9f45bf6949..fe9e590b80 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
static int split_commit_in_progress(struct wt_status *s)
{
int split_in_progress = 0;
- char *head, *orig_head, *rebase_amend, *rebase_orig_head;
+ struct object_id head_oid, orig_head_oid;
+ char *rebase_amend, *rebase_orig_head;
if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
!s->branch || strcmp(s->branch, "HEAD"))
return 0;
- head = read_line_from_git_path("HEAD");
- orig_head = read_line_from_git_path("ORIG_HEAD");
+ if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
+ read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
+ return 0;
+
rebase_amend = read_line_from_git_path("rebase-merge/amend");
rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
- if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+ if (!rebase_amend || !rebase_orig_head)
; /* fall through, no split in progress */
else if (!strcmp(rebase_amend, rebase_orig_head))
- split_in_progress = !!strcmp(head, rebase_amend);
- else if (strcmp(orig_head, rebase_orig_head))
+ split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
+ else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
split_in_progress = 1;
- free(head);
- free(orig_head);
free(rebase_amend);
free(rebase_orig_head);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 0/4] refs: improve handling of special refs
From: Patrick Steinhardt @ 2023-12-12 7:18 UTC (permalink / raw)
To: git; +Cc: Taylor Blau, Phillip Wood
In-Reply-To: <cover.1701243201.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4823 bytes --]
Hi,
this is the second version of my patch series that aims to improve our
handling of special refs. This patch series is in preparation for the
upcoming reftable backend.
Changes compared to v1:
- Patch 1: Stop needlessly resetting `errno` before calling
`strbuf_read_file()`. Also, clean up state we create in our test
repos properly.
- Patch 3: The discussion with Phillip made me reevaluate my claim
that "rebase-apply/" and "rebase-merge/" contain special refs.
Indeed they don't, all files in there seem to be exclusively
read and written via the filesystem without ever going via the
refdb. I've thus dropped them from the set of special refs.
- Patch 4: The array of static refs is now static.
Thanks for your reviews!
Patrick
Patrick Steinhardt (4):
wt-status: read HEAD and ORIG_HEAD via the refdb
refs: propagate errno when reading special refs fails
refs: complete list of special refs
bisect: consistently write BISECT_EXPECTED_REV via the refdb
bisect.c | 25 +++-------------
builtin/bisect.c | 8 ++---
refs.c | 59 +++++++++++++++++++++++++++++++++++--
t/t1403-show-ref.sh | 10 +++++++
t/t6030-bisect-porcelain.sh | 2 +-
wt-status.c | 17 ++++++-----
6 files changed, 82 insertions(+), 39 deletions(-)
Range-diff against v1:
1: 35b74eb972 = 1: 1db3eb3945 wt-status: read HEAD and ORIG_HEAD via the refdb
2: 691552a17e ! 2: 24032a62e5 refs: propagate errno when reading special refs fails
@@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
- if (strbuf_read_file(&content, full_path.buf, 0) < 0)
-+ errno = 0;
-+
+ if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
+ *failure_errno = errno;
goto done;
@@ t/t1403-show-ref.sh: test_expect_success '--exists with directory fails with gen
+'
+
+test_expect_success '--exists with existing special ref' '
++ test_when_finished "rm .git/FETCH_HEAD" &&
+ git rev-parse HEAD >.git/FETCH_HEAD &&
+ git show-ref --exists FETCH_HEAD
+'
3: 0e38103114 ! 3: 3dd9089fd5 refs: complete list of special refs
@@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
+ * - MERGE_HEAD may contain multiple object IDs when merging multiple
+ * heads.
+ *
-+ * - "rebase-apply/" and "rebase-merge/" contain all of the state for
-+ * rebases, where keeping it closely together feels sensible.
-+ *
+ * There are some exceptions that you might expect to see on this list
+ * but which are handled exclusively via the reference backend:
+ *
+ * - CHERRY_PICK_HEAD
++ *
+ * - HEAD
++ *
+ * - ORIG_HEAD
+ *
++ * - "rebase-apply/" and "rebase-merge/" contain all of the state for
++ * rebases, including some reference-like files. These are
++ * exclusively read and written via the filesystem and never go
++ * through the refdb.
++ *
+ * Writing or deleting references must consistently go either through
+ * the filesystem (special refs) or through the reference backend
+ * (normal ones).
+ */
-+ const char * const special_refs[] = {
++ static const char * const special_refs[] = {
+ "AUTO_MERGE",
+ "BISECT_EXPECTED_REV",
+ "FETCH_HEAD",
+ "MERGE_AUTOSTASH",
+ "MERGE_HEAD",
+ };
-+ int i;
++ size_t i;
+
+ for (i = 0; i < ARRAY_SIZE(special_refs); i++)
+ if (!strcmp(refname, special_refs[i]))
+ return 1;
+
-+ /*
-+ * git-rebase(1) stores its state in `rebase-apply/` or
-+ * `rebase-merge/`, including various reference-like bits.
-+ */
-+ if (starts_with(refname, "rebase-apply/") ||
-+ starts_with(refname, "rebase-merge/"))
-+ return 1;
-+
+ return 0;
+}
+
4: c7ab26fb7e ! 4: 4a4447a3f5 bisect: consistently write BISECT_EXPECTED_REV via the refdb
@@ refs.c: static int is_special_ref(const char *refname)
* but which are handled exclusively via the reference backend:
*
+ * - BISECT_EXPECTED_REV
++ *
* - CHERRY_PICK_HEAD
+ *
* - HEAD
- * - ORIG_HEAD
@@ refs.c: static int is_special_ref(const char *refname)
*/
- const char * const special_refs[] = {
+ static const char * const special_refs[] = {
"AUTO_MERGE",
- "BISECT_EXPECTED_REV",
"FETCH_HEAD",
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ 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