* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Junio C Hamano @ 2023-10-04 8:20 UTC (permalink / raw)
To: Sergey Organov; +Cc: git
In-Reply-To: <874jj7lh7x.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> I believe I've addressed this in details in my reply here:
>>> <87o7hok8dx.fsf@osv.gnss.ru>, and got no further objections from you
>>> since then, so I figure I'd ask to finally let the patch in.
>>
>> You need to know that no response does not mean no objection. You
>> repeated why the less useful combination is what you want, but that
>> does not mean the combination deserves to squat on short-and-sweet
>> 'd' and prevent others from coming up with a better use for it.
>
> Yep, but I've asked what's better use for -d than "get me diff"? Do you
> really have an idea?
The primary point is to leave it open for future developers.
If I have to pick a candidate for "get me diff" that is the most
useful among those currently are available, it is "give patches to
all single-parent commit, and show tricky conflict resolution part
only for merge commits". Before "--remerge-diff" was invented, my
answer would have been "give patches to all single-parent commit,
and show combined diff in the compact form for merge commits", aka
"git log --cc". Even though we did not know if a better output
presentation for merge commits would be coming, we did not let it
squat on any short-and-sweet single letter synonym.
^ permalink raw reply
* [PATCH v3] git-status.txt: fix minor asciidoc format issue
From: cousteau via GitGitGadget @ 2023-10-04 2:22 UTC (permalink / raw)
To: git; +Cc: Javier Mora, cousteau, Javier Mora
In-Reply-To: <pull.1591.v2.git.1696350802820.gitgitgadget@gmail.com>
From: Javier Mora <cousteaulecommandant@gmail.com>
The list of additional XY values for submodules in short format
isn't formatted consistently with the rest of the document.
Format as list for consistency.
Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
---
git-status.txt: minor asciidoc format correction
The paragraph below the list of short option combinations was hard to
read; turns out it wasn't correctly formatted in asciidoc.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1591%2Fcousteaulecommandant%2Fman-git-status-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1591/cousteaulecommandant/man-git-status-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1591
Range-diff vs v2:
1: 811885a275f ! 1: 819499eb4c8 git-status.txt: fix minor asciidoc format issue
@@ Metadata
## Commit message ##
git-status.txt: fix minor asciidoc format issue
- The paragraph below the list of short option combinations
- isn't correctly formatted, making the result hard to read.
+ The list of additional XY values for submodules in short format
+ isn't formatted consistently with the rest of the document.
+ Format as list for consistency.
Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
Documentation/git-status.txt | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index b27d127b5e2..48f46eb2047 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -246,10 +246,9 @@ U U unmerged, both modified
Submodules have more state and instead report
- M the submodule has a different HEAD than
- recorded in the index
- m the submodule has modified content
- ? the submodule has untracked files
+* 'M' = the submodule has a different HEAD than recorded in the index
+* 'm' = the submodule has modified content
+* '?' = the submodule has untracked files
since modified content or untracked files in a submodule cannot be added
via `git add` in the superproject to prepare a commit.
base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2] git-status.txt: fix minor asciidoc format issue
From: Javier Mora @ 2023-10-04 2:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: cousteau via GitGitGadget, git
In-Reply-To: <xmqq4jj7zc5b.fsf@gitster.g>
Fair enough; I'll update the commit message. I just thought it still
worked more or less so I left it as is.
El mar, 3 oct 2023 a las 21:25, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> "cousteau via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Javier Mora <cousteaulecommandant@gmail.com>
> >
> > The paragraph below the list of short option combinations
> > isn't correctly formatted, making the result hard to read.
> >
> > Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
> > ---
>
> The above probably no longer describes the situation the patch
> intends to correct, I suspect. It used to be near-impossible hard
> to read, but at least with them indented they are legible.
>
> The additional states for submodules are typeset differently
> from how the states for paths for normal blobs are listed as
> enumeration. Format them in the same way for consistency.
>
> or something like that, perhaps.
>
> > Documentation/git-status.txt | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> > index b27d127b5e2..48f46eb2047 100644
> > --- a/Documentation/git-status.txt
> > +++ b/Documentation/git-status.txt
> > @@ -246,10 +246,9 @@ U U unmerged, both modified
> >
> > Submodules have more state and instead report
> >
> > - M the submodule has a different HEAD than
> > - recorded in the index
> > - m the submodule has modified content
> > - ? the submodule has untracked files
> > +* 'M' = the submodule has a different HEAD than recorded in the index
> > +* 'm' = the submodule has modified content
> > +* '?' = the submodule has untracked files
> >
> > since modified content or untracked files in a submodule cannot be added
> > via `git add` in the superproject to prepare a commit.
>
> Thanks for making this part of the documentation better.
>
^ permalink raw reply
* Is SANITIZE=leak make test unreliable for anyone else?
From: Eric W. Biederman @ 2023-10-04 1:33 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
Jeff,
Please accept my apologies for slightly hijacking your posting, but I
see you have been fixing some leaks, and so presumably you are familiar
with building git with "SANITIZE=leak".
I have fixed some leaks in my SHA1+SHA256 patchset recently and while
tracking them down I found that simply enabling SANITIZE=leak caused
"make test" on git v2.42 without patches to give different failures
from test run to test run.
Well actually I wound up with the following command line:
GIT_TEST_PASSING_SANITIZE_LEAK=true GIT_TEST_SANITIZE_LEAK_LOG=true SANITIZE=leak DEVELOPER=1 make test
I had removed "-j32" to make things more reproducible.
I observed this unreliability with SANITIZE=leak when building git on
an fully updated version of debian 12.
My big question is:
Do other people see random test failures when SANITIZE=leak is enabled?
Is it just me?
Thanks,
Eric
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Eric W. Biederman @ 2023-10-04 1:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric W. Biederman
In-Reply-To: <xmqqedic35u4.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> * eb/hash-transition (2023-10-02) 30 commits
> - t1016-compatObjectFormat: add tests to verify the conversion between objects
> - t1006: test oid compatibility with cat-file
> - t1006: rename sha1 to oid
> - test-lib: compute the compatibility hash so tests may use it
> - builtin/ls-tree: let the oid determine the output algorithm
> - object-file: handle compat objects in check_object_signature
> - tree-walk: init_tree_desc take an oid to get the hash algorithm
> - builtin/cat-file: let the oid determine the output algorithm
> - rev-parse: add an --output-object-format parameter
> - repository: implement extensions.compatObjectFormat
> - object-file: update object_info_extended to reencode objects
> - object-file-convert: convert commits that embed signed tags
> - object-file-convert: convert commit objects when writing
> - object-file-convert: don't leak when converting tag objects
> - object-file-convert: convert tag objects when writing
> - object-file-convert: add a function to convert trees between algorithms
> - object: factor out parse_mode out of fast-import and tree-walk into in object.h
> - cache: add a function to read an OID of a specific algorithm
> - tag: sign both hashes
> - commit: export add_header_signature to support handling signatures on tags
> - commit: convert mergetag before computing the signature of a commit
> - commit: write commits for both hashes
> - object-file: add a compat_oid_in parameter to write_object_file_flags
> - object-file: update the loose object map when writing loose objects
> - loose: compatibilty short name support
> - loose: add a mapping between SHA-1 and SHA-256 for loose objects
> - repository: add a compatibility hash algorithm
> - object-names: support input of oids in any supported hash
> - oid-array: teach oid-array to handle multiple kinds of oids
> - object-file-convert: stubs for converting from one object format to another
>
> Teach a repository to work with both SHA-1 and SHA-256 hash algorithms.
>
> Breaks a few CI jobs when merged to 'seen'.
> cf. <xmqqbkdmjbkp.fsf@gitster.g>
I see that you have picked up the v2 version. Thank you.
I pushed v2 out precisely because it contains fixes that should have
fixed all of the CI breakages.
I am not really familiar with github but looking at the recent CI runs
it appears since v2 landed the seen branch has been building cleanly.
I haven't misread something have I?
I just don't want people to avoid reviewing it because it is that huge
patchset that causes problems in seen.
Eric
^ permalink raw reply
* Re: Possible to update-ref remote repository?
From: Junio C Hamano @ 2023-10-04 1:13 UTC (permalink / raw)
To: Jeff King; +Cc: Jesse Hopkins, git
In-Reply-To: <20231003200018.GB1562@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> All that said, I do think it might be reasonable for git-push to support
> this directly.
Yup. It certainly is simpler if you can leverage existing helpers.
It will become even simpler in a reasonably modularlized world that
hopefully may materialize before we all retire ;-). I am hoping
that some of the folks who are interested in and talking about
libification can be fooled into doing the necessary work to
introduce proper abstraction, in addition to whatever they are
doing.
Wouldn't it be great if you can have an in-core repository object,
that knows what its object store is, has an index_state object that
is tied to that object store, has a reference database whose values
point into the object store, and if you can choose and mix these
repository components' implementations? If done right, parts of the
above set of components can be replaced with mock implementations
that are in-core only.
To run "git push --repoint-only there 01beef23:master", you should
be able to start your process totally outside an repository, yet
create an in-core-only repository instance with an in-core-only
object store instance, and because you took the object name to push
on the command line, your in-core object store can "lie" to a call
"create an in-core object for this SHA-1" by returning a fake
in-core commit object, and your in-core-only ref database has that
commit pointed at by some ref. Then because higher level "client"
code to walk revisions, enumerate refs, etc., would all implement
what they need to do by calling vtable of these in-core objects, you
can do the "repoint-only" push without being in any repository, as
such an implementation would not touch any filesystem (you can then
plug in different implementation of object store etc., and even make
them perform reasonably well if you manage to do the abstraction
right).
But that would probably be at least 6 months away, even if we had a
handful of competent developers totally dedicated to the effort
without any distraction, which I do not know how likely it is to
happen.
^ permalink raw reply
* Re: [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch}
From: Junio C Hamano @ 2023-10-04 1:10 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>
"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
> The commands need a path to a submodule but treated it as the name when
> modifying the .gitmodules file, leading to confusion when a submodule's
> name does not match its path.
Thanks for noticing and fixing this common mix-up.
> Because calling submodule_from_path initializes the submodule cache, we
> need to manually trigger a reread before syncing, as the cache is
> missing the config change we just made.
> Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f6871efd95..f376466a5e 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2902,19 +2902,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
> N_("git submodule set-url [--quiet] <path> <newurl>"),
> NULL
> };
> + const struct submodule *sub;
>
> argc = parse_options(argc, argv, prefix, options, usage, 0);
>
> if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1]))
> usage_with_options(usage, options);
>
> - config_name = xstrfmt("submodule.%s.url", path);
> + sub = submodule_from_path(the_repository, null_oid(), path);
>
> + if (!sub)
> + die(_("no submodule mapping found in .gitmodules for path '%s'"),
> + path);
> +
> + config_name = xstrfmt("submodule.%s.url", sub->name);
Looks correct.
> config_set_in_gitmodules_file_gently(config_name, newurl);
> - sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0);
> +
> + repo_read_gitmodules (the_repository, 0);
Style. No extra space between function name and "(".
But more importantly, is this sufficient? repo_read_gitmodules()
does not seem to clear the cache and build its contents from scratch
(as submodule_cache_check_init() bypasses itself upon second call).
The code is correct because submodule-config.c::config_from() does
set .overwrite to true, so submodule.name.url would be overwritten
to the new value, I think, but somebody else who is more familiar
with the recent submodule code may want to sanity check my analysis.
> + sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0);
Is the use of "sub" still safe here?
I think it is safe as repo_read_gitmodules() did not rebuild the
in-core cache from scratch but did selective overwrite, so the
in-core instance "sub" is still valid, but again somebody else who
is more familiar with the recent submodule code may want to sanity
check.
> @@ -2942,19 +2949,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
> N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"),
> NULL
> };
> + const struct submodule *sub;
>
> argc = parse_options(argc, argv, prefix, options, usage, 0);
>
> if (!opt_branch && !opt_default)
> die(_("--branch or --default required"));
>
> if (opt_branch && opt_default)
> die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default");
>
> if (argc != 1 || !(path = argv[0]))
> usage_with_options(usage, options);
>
> - config_name = xstrfmt("submodule.%s.branch", path);
> + sub = submodule_from_path(the_repository, null_oid(), path);
> +
> + if (!sub)
> + die(_("no submodule mapping found in .gitmodules for path '%s'"),
> + path);
> +
> + config_name = xstrfmt("submodule.%s.branch", sub->name);
> ret = config_set_in_gitmodules_file_gently(config_name, opt_branch);
This side happens not to require re-reading of gitmodules file,
because, unlike the URL helper, we do not care what we have in the
in-core cache is stale. It is correct but feels a bit brittle.
^ permalink raw reply
* Re: [PATCH 3/6] t7419: Actually test the branch switching
From: Junio C Hamano @ 2023-10-04 0:20 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
In-Reply-To: <20231003185047.2697995-3-heftig@archlinux.org>
"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
> Subject: Re: [PATCH 3/6] t7419: Actually test the branch switching
"Actually" -> "actually".
> The submodule repo the test set up had the 'topic' branch checked out,
> meaning the repo's default branch (HEAD) is the 'topic' branch.
>
> The following tests then pretended to switch between the default branch
> and the 'topic' branch. This was papered over by continually adding
> commits to the 'topic' branch and checking if the submodule gets updated
> to this new commit.
>
> Return the submodule repo to the 'main' branch after setup so we can
> actually test the switching behavior.
Nicely spotted.
> diff --git a/t/t7419-submodule-set-branch.sh b/t/t7419-submodule-set-branch.sh
> index 232065504c..5ac16d0eb7 100755
> --- a/t/t7419-submodule-set-branch.sh
> +++ b/t/t7419-submodule-set-branch.sh
> @@ -11,23 +11,28 @@ as expected.
>
> TEST_PASSES_SANITIZE_LEAK=true
> TEST_NO_CREATE_REPO=1
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
And it is good that this variable is used to prepare the test for
both kinds of CI runs that use 'main' or 'master' as the default
branch.
^ permalink raw reply
* Re: [Outreachy] Move existing tests to a unit testing framework
From: Luma @ 2023-10-03 23:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqedibzgi1.fsf@gitster.g>
oh yes, "Move existing tests to a unit testing framework" was the
only listed project for this current Outreachy cohort. So, I used it
to express my intent.
I appreciate the clarification on authorship identity for patches. I
will update subsequent patches with a legal full name to conform to
the community rules.
Regards.
On Tue, Oct 3, 2023 at 7:51 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Luma <ach.lumap@gmail.com> writes:
>
> > Hi;
> > My name is Luma, and I wanted to take a moment to introduce myself
> > and share some
> > insights on an essential aspect of avoiding pipes in git related
> > commands in test scripts.
> >
> > I am an outreachy applicant for the December 2023 cohort and look
> > forward to learning from you.
>
> I notice that the title of the message and the immediate topic you
> discuss in the body of the message do not match. I presume that the
> topic on the title is what you prefer to work on if the unit testing
> framework is ready by the time Outreachy program starts, and the
> mention about "do not clobber exit code of Git with pipes in the
> tests" is your "dip the tip of a toe in water" microproject?
>
> Welcome to the Git development community.
>
> Do you have a single word name? If so please disregard the below,
> but in case "Luma" is just a nickname (e.g. like I am introducing
> myself to my Git friends "Hi, I am Gitster!") you use online, please
> read on.
>
> For signing off your patches, we'd prefer to see your real name
> used, as Signed-off-by: is meant to have legal significance. And
> because we also expect the authorship identity to match the
> name/e-mail of the sign-off, it would mean your patch submissions
> are expected to look like:
>
> From: Luma <ach.lumap@gmail.com>
> Subject: ... title of the patch goes here ...
>
> ... body of the proposed commit log message goes here...
>
> Signed-off-by: Luma <ach.lumap@gmail.com>
>
> but "Luma" replaced with your full real name.
>
> Thanks.
^ permalink raw reply
* Re: [PATCH 2/6] submodule--helper: return error from set-url when modifying failed
From: Junio C Hamano @ 2023-10-03 23:10 UTC (permalink / raw)
To: Jan Alexander Steffens (heftig); +Cc: git
In-Reply-To: <20231003185047.2697995-2-heftig@archlinux.org>
"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes:
> set-branch will return an error when setting the config fails so I don't
> see why set-url shouldn't. Also skip the sync in this case.
Two other failures in this helper function gives end-user readable
message (parameter errors are greeted with usage, giving wrong path
is greeted with a die()), but this new error condition will silently
die unless config_set_in_gitmodules_file_gently() complains.
I wonder if we should give an error message in this case, too.
In any case, not calling sync after failed set_in_gitmodules is a
sensible design decision.
^ permalink raw reply
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Junio C Hamano @ 2023-10-03 21:57 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <xmqqwmw3wgeo.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> I wonder if this would be a safer alternative, or is it doing too
> much more than what is necessary?
An alternative with much smaller blast radius would be to do this.
Hopefully, by going "$(pwd)/." before asking the value returned by
the `pwd` command, we can make sure that the trailing slash is
removed (or at least $(pwd) and $TEST_DIRECTORY should be identical
after this is done).
t/test-lib.sh | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git c/t/test-lib.sh w/t/test-lib.sh
index 1656c9eed0..d159358b21 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -19,9 +19,13 @@
# t/ subdirectory and are run in 'trash directory' subdirectory.
if test -z "$TEST_DIRECTORY"
then
- # ensure that TEST_DIRECTORY is an absolute path so that it
+ # Ensure that TEST_DIRECTORY is an absolute path so that it
# is valid even if the current working directory is changed
- TEST_DIRECTORY=$(pwd)
+ # Some environments can talk the shell into keeping trailing
+ # slash in $PWD---go there and ask where we are to work it
+ # around, as we expect TEST_DIRECTORY and PWD are both
+ # canonical and can textually be compared for equality
+ TEST_DIRECTORY=$(cd "$(pwd)/." && pwd)
else
# The TEST_DIRECTORY will always be the path to the "t"
# directory in the git.git checkout. This is overridden by
^ permalink raw reply related
* [PATCH v2] builtin/repack.c: avoid making cruft packs preferred
From: Taylor Blau @ 2023-10-03 21:54 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <19d9aae08eab05c6b5dda4c2090236b1c3f62998.1696349955.git.me@ttaylorr.com>
When doing a `--geometric` repack, we make sure that the preferred pack
(if writing a MIDX) is the largest pack that we *didn't* repack. That
has the effect of keeping the preferred pack in sync with the pack
containing a majority of the repository's reachable objects.
But if the repository happens to double in size, we'll repack
everything. Here we don't specify any `--preferred-pack`, and instead
let the MIDX code choose.
In the past, that worked fine, since there would only be one pack to
choose from: the one we just wrote. But it's no longer necessarily the
case that there is one pack to choose from. It's possible that the
repository also has a cruft pack, too.
If the cruft pack happens to come earlier in lexical order (and has an
earlier mtime than any non-cruft pack), we'll pick that pack as
preferred. This makes it impossible to reuse chunks of the reachable
pack verbatim from pack-objects, so is sub-optimal.
Luckily, this is a somewhat rare circumstance to be in, since we would
have to repack the entire repository during a `--geometric` repack, and
the cruft pack would have to sort ahead of the pack we just created.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
A small re-roll of the original patch, removing a few paragraphs from
the proposed log message that are irrelevant and impossible to produce
in today's git.git.
For more details, see <ZRyNHRf+tQwV+T6P@nand.local>.
Range-diff against v1:
1: 19d9aae08e ! 1: 0353939351 builtin/repack.c: avoid making cruft packs preferred
@@ Commit message
have to repack the entire repository during a `--geometric` repack, and
the cruft pack would have to sort ahead of the pack we just created.
- Note that this behavior is usually just a performance regression. But
- it's possible it could be a correctness issue.
-
- Suppose an object was duplicated among the cruft and non-cruft pack. The
- MIDX will pick the one from the pack with the lowest mtime, which will
- always be the cruft one. But if the non-cruft pack happens to sort
- earlier in lexical order, we'll treat that one as preferred, but not all
- duplicates will be resolved in favor of that pack.
-
- So if we happened to have an object which appears in both packs
- (e.g., due to a cruft object being freshened, causing it to appear
- loose, and then repacking it via the `--geometric` repack) it's possible
- the duplicate would be picked from the non-preferred pack.
-
Signed-off-by: Taylor Blau <me@ttaylorr.com>
## builtin/repack.c ##
builtin/repack.c | 47 ++++++++++++++++++++++++++++++++++++++++-
t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 04770b15fe..a1a893d952 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -355,6 +355,18 @@ static struct generated_pack_data *populate_pack_exts(const char *name)
return data;
}
+static int has_pack_ext(const struct generated_pack_data *data,
+ const char *ext)
+{
+ int i;
+ for (i = 0; i < ARRAY_SIZE(exts); i++) {
+ if (strcmp(exts[i].name, ext))
+ continue;
+ return !!data->tempfiles[i];
+ }
+ BUG("unknown pack extension: '%s'", ext);
+}
+
static void repack_promisor_objects(const struct pack_objects_args *args,
struct string_list *names)
{
@@ -772,6 +784,7 @@ static void midx_included_packs(struct string_list *include,
static int write_midx_included_packs(struct string_list *include,
struct pack_geometry *geometry,
+ struct string_list *names,
const char *refs_snapshot,
int show_progress, int write_bitmaps)
{
@@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,
if (preferred)
strvec_pushf(&cmd.args, "--preferred-pack=%s",
pack_basename(preferred));
+ else if (names->nr) {
+ /* The largest pack was repacked, meaning that either
+ * one or two packs exist depending on whether the
+ * repository has a cruft pack or not.
+ *
+ * Select the non-cruft one as preferred to encourage
+ * pack-reuse among packs containing reachable objects
+ * over unreachable ones.
+ *
+ * (Note we could write multiple packs here if
+ * `--max-pack-size` was given, but any one of them
+ * will suffice, so pick the first one.)
+ */
+ for_each_string_list_item(item, names) {
+ struct generated_pack_data *data = item->util;
+ if (has_pack_ext(data, ".mtimes"))
+ continue;
+
+ strvec_pushf(&cmd.args, "--preferred-pack=pack-%s.pack",
+ item->string);
+ break;
+ }
+ } else {
+ /*
+ * No packs were kept, and no packs were written. The
+ * only thing remaining are .keep packs (unless
+ * --pack-kept-objects was given).
+ *
+ * Set the `--preferred-pack` arbitrarily here.
+ */
+ ;
+ }
if (refs_snapshot)
strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
@@ -1360,7 +1405,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
struct string_list include = STRING_LIST_INIT_NODUP;
midx_included_packs(&include, &existing, &names, &geometry);
- ret = write_midx_included_packs(&include, &geometry,
+ ret = write_midx_included_packs(&include, &geometry, &names,
refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
show_progress, write_bitmaps > 0);
diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh
index dc86ca8269..be3735dff0 100755
--- a/t/t7704-repack-cruft.sh
+++ b/t/t7704-repack-cruft.sh
@@ -372,4 +372,43 @@ test_expect_success '--max-cruft-size ignores non-local packs' '
)
'
+test_expect_success 'reachable packs are preferred over cruft ones' '
+ repo="cruft-preferred-packs" &&
+ git init "$repo" &&
+ (
+ cd "$repo" &&
+
+ # This test needs to exercise careful control over when a MIDX
+ # is and is not written. Unset the corresponding TEST variable
+ # accordingly.
+ sane_unset GIT_TEST_MULTI_PACK_INDEX &&
+
+ test_commit base &&
+ test_commit --no-tag cruft &&
+
+ non_cruft="$(echo base | git pack-objects --revs $packdir/pack)" &&
+ # Write a cruft pack which both (a) sorts ahead of the non-cruft
+ # pack in lexical order, and (b) has an older mtime to appease
+ # the MIDX preferred pack selection routine.
+ cruft="$(echo pack-$non_cruft.pack | git pack-objects --cruft $packdir/pack-A)" &&
+ test-tool chmtime -1000 $packdir/pack-A-$cruft.pack &&
+
+ test_commit other &&
+ git repack -d &&
+
+ git repack --geometric 2 -d --write-midx --write-bitmap-index &&
+
+ # After repacking, there are two packs left: one reachable one
+ # (which is the result of combining both of the existing two
+ # non-cruft packs), and one cruft pack.
+ find .git/objects/pack -type f -name "*.pack" >packs &&
+ test_line_count = 2 packs &&
+
+ # Make sure that the pack we just wrote is marked as preferred,
+ # not the cruft one.
+ pack="$(test-tool read-midx --preferred-pack $objdir)" &&
+ test_path_is_missing "$packdir/$(basename "$pack" ".idx").mtimes"
+ )
+'
+
test_done
--
2.42.0.341.g9d86323b73.dirty
^ permalink raw reply related
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Taylor Blau @ 2023-10-03 21:52 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Patrick Steinhardt
In-Reply-To: <20231003201617.GE1562@coredump.intra.peff.net>
On Tue, Oct 03, 2023 at 04:16:17PM -0400, Jeff King wrote:
> On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
>
> > Note that this behavior is usually just a performance regression. But
> > it's possible it could be a correctness issue.
> >
> > Suppose an object was duplicated among the cruft and non-cruft pack. The
> > MIDX will pick the one from the pack with the lowest mtime, which will
> > always be the cruft one. But if the non-cruft pack happens to sort
> > earlier in lexical order, we'll treat that one as preferred, but not all
> > duplicates will be resolved in favor of that pack.
> >
> > So if we happened to have an object which appears in both packs
> > (e.g., due to a cruft object being freshened, causing it to appear
> > loose, and then repacking it via the `--geometric` repack) it's possible
> > the duplicate would be picked from the non-preferred pack.
>
> I'm not sure I understand how that is a correctness issue. The contents
> of the object are the same in either pack. Or do you mean that the
> pack-reuse code in pack-objects.c may get confused and try to use the
> wrong pack/offset when sending the object out? I would think it would
> always be coming from the preferred pack for that code (so the outcome
> is just that we fail to do the pack-reuse optimization very well, but we
> don't generate a wrong answer).
Admittedly I'm not 100% sure of my interpretation here either, since I
wrote most of this patch's log message nearly two years ago ;-).
I think the issue was as follows:
- you have an object duplicated among some cruft and non-cruft pack
- the cruft pack happens to have an older mtime than the non-cruft one
- the repack reused no non-cruft packs, which (before this patch)
would let the MIDX avoid picking a preferred pack
- if the non-cruft pack happens to sort ahead of the cruft one in
lexical order, it'll appear in the first few bits of the bitmap's
ordering, causing us to treat it as if it were the preferred pack
- ...but the MIDX will resolve duplicate objects in favor of the
oldest pack when neither are preferred, causing us to pick a
duplicate from the non-"preferred" pack.
(The last point is what causes the pack-bitmap reading code to get
confused, since it expects and makes assumption based on resolving ties
in favor of the preferred pack.)
That feels right to me, but admittedly my memory is pretty hazy on those
bitmap bugs. However, I don't think that this is an issue anymore, since
this patch was written before we had 177c0d6e63 (midx: infer preferred
pack when not given one, 2021-08-31) in GitHub's private fork, and the
patch message here incorrectly refers to statements about GitHub's fork,
not git.git.
But since we have 177c0d6e63 in git.git, this paragraph can and should
be dropped as finding ourselves in that situation is impossible, since
we would infer a preferred pack when not given one, and resolve
duplicates in favor of it.
That makes this purely a performance issue.
> Other than that, the explanation and patch make perfect sense to me, and
> the patch looks good.
Thanks!
> -Peff
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] builtin/repack.c: avoid making cruft packs preferred
From: Taylor Blau @ 2023-10-03 21:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Patrick Steinhardt
In-Reply-To: <xmqqa5szzcdz.fsf@gitster.g>
On Tue, Oct 03, 2023 at 01:20:24PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Tue, Oct 03, 2023 at 12:27:51PM -0400, Taylor Blau wrote:
> >> I've had this sitting in my patch queue for a while now. It's a
> >> non-critical performance fix that avoids the repack/MIDX machinery from
> >> ever choosing a cruft pack as preferred when writing a MIDX bitmap
> >> without a given --preferred-pack.
> >>
> >> There is no correctness issue here, but choosing a pack with few/no
> >> reachable objects means that our pack reuse mechanism will rarely kick
> >> in, resulting in performance degradation.
> >>
> >> builtin/repack.c | 47 ++++++++++++++++++++++++++++++++++++++++-
> >> t/t7704-repack-cruft.sh | 39 ++++++++++++++++++++++++++++++++++
> >> 2 files changed, 85 insertions(+), 1 deletion(-)
> >
> > Oops, I should have mentioned that this is meant to be applied on top of
> > 'tb/multi-cruft-pack' to reduce the conflict resolution burden. Sorry
> > about that.
>
> Sorry, but I do not follow. tb/multi-cruft-pack was merged to
> 'master' at c0b5d46d (Documentation/gitformat-pack.txt: drop mixed
> version section, 2023-08-28) but back then t7704 did not exist. Do
> you mean the other topic in-flight from you about max-cruft-size?
My mistake -- this should be applied on top of the patches at this
series:
https://lore.kernel.org/git/cover.1696293862.git.me@ttaylorr.com/
which is the other topic that you're referring to. I ran "git
for-each-ref 'refs/remotes/origin/tb/*' | grep cruft" and mistakenly
grabbed 'tb/multi-cruft-pack' thinking that that was that series and not
the one already merged to 'master'.
But it looks like the series linked above hasn't yet grown a topic
branch in gitster/git yet, hence my mistake. Sorry about that!
Thanks,
Taylor
^ permalink raw reply
* Re: [silly] loose, pack, and another thing?
From: Junio C Hamano @ 2023-10-03 21:26 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git
In-Reply-To: <20231003190955.GA1562@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> One thing that scares me about a regular "ln" between the worktree and
> odb is that you are very susceptible to corrupting the repository by
> modifying the worktree file with regular tools. If they do a complete
> rewrite and atomic rename (or link) to put the new file in place, that
> is OK. But opening the file for appending, or general writing, is bad.
Very true.
> You can get some safety with the immutable attribute (which applies to
> the inode itself, and thus any path that hardlinks to it). But setting
> that usually requires being root. And it creates other irritations for
> normal use (you have to unset it before even removing the hardlink).
As a regular user, "chmod a-w" has the same characteristics (works
at the inode level) but without "cannot remove it" downside. It
used to be sufficient in RCS and CVS days, though, as a signal that
you are only to look at it without touching it, to "chmod a-w" a
path that is checked out but not for modifying. Some editors even
offer to do chmod u+w for you when saving, so if we want absolute
safety, it may not be enough.
> It would be nice if there was some portable copy-on-write abstraction we
> could rely on, but I don't think there is one.
;-)
^ permalink raw reply
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Junio C Hamano @ 2023-10-03 21:21 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003082323.3002663-1-stepnem@smrk.net>
Štěpán Němec <stepnem@smrk.net> writes:
> Turns out having `pwd` (which TEST_DIRECTORY defaults to) print $PWD
> with a trailing slash isn't very difficult, in my case it went something
> like
>
> ; tmux new-window -c ~/src/git/t/
> [in the new window]
> ; sh ./t0000-basic.sh
> PANIC: Running in a /home/stepnem/src/git/t/ that doesn't end in '/t'?
> ; pwd
> /home/stepnem/src/git/t/
>
> (tmux(1) apparently sets PWD in the environment in addition to calling
> chdir(2), which seems enough to make at least some shells preserve the
> trailing slash in `pwd` output.)
>
> Strip the trailing slash, if present, to prevent bailing out with the
> PANIC message in such cases.
>
> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
> ---
> t/test-lib.sh | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1656c9eed006..3b6f1a17e349 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -35,6 +35,7 @@ else
> # needing to exist.
> TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
> fi
> +TEST_DIRECTORY="${TEST_DIRECTORY%/}"
> if test -z "$TEST_OUTPUT_DIRECTORY"
> then
> # Similarly, override this to store the test-results subdir
>
> base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
While this would certainly squelch the particular test on TEST_DIRECTORY
I am not sure if a safer fix would be to fix your PWD when it has a
trailing slash. Your tests with this patch may be taking unintended
branch when they do something like
if test "$TRASH_DIRECTORY" = "$(pwd)"
then
...
fi
as TRASH_DIRECTORY, whose default is derived from TEST_DIRECTORY and
thanks to your fix it now does not have an extra slash in it, does
not have a trailing slash but your $(pwd) or $PWD would have the
trailing slash, simply because the patch only fixes TEST_DIRECTORY
without fixing PWD.
I wonder if this would be a safer alternative, or is it doing too
much more than what is necessary?
t/test-lib.sh | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git c/t/test-lib.sh w/t/test-lib.sh
index 1656c9eed0..6ec42eab0d 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -22,19 +22,26 @@ then
# ensure that TEST_DIRECTORY is an absolute path so that it
# is valid even if the current working directory is changed
TEST_DIRECTORY=$(pwd)
-else
- # The TEST_DIRECTORY will always be the path to the "t"
- # directory in the git.git checkout. This is overridden by
- # e.g. t/lib-subtest.sh, but only because its $(pwd) is
- # different. Those tests still set "$TEST_DIRECTORY" to the
- # same path.
- #
- # See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for
- # hard assumptions about "$GIT_BUILD_DIR/t" existing and being
- # the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper"
- # needing to exist.
- TEST_DIRECTORY=$(cd "$TEST_DIRECTORY" && pwd) || exit 1
fi
+
+# The TEST_DIRECTORY will always be the path to the "t"
+# directory in the git.git checkout. This is overridden by
+# e.g. t/lib-subtest.sh, but only because its $(pwd) is
+# different. Those tests still set "$TEST_DIRECTORY" to the
+# same path.
+#
+# See use of "$GIT_BUILD_DIR" and "$TEST_DIRECTORY" below for
+# hard assumptions about "$GIT_BUILD_DIR/t" existing and being
+# the "$TEST_DIRECTORY", and e.g. "$TEST_DIRECTORY/helper"
+# needing to exist.
+#
+# Also, apparently a shell spawned in some tricky way can be
+# talked into keeping a slash at the end of its $PWD.
+#
+# All of the above can be worked around by doing an extra chdir
+# and asking where we ended up to be.
+TEST_DIRECTORY=$(cd "$TEST_DIRECTORY/." && pwd) || exit 1
+
if test -z "$TEST_OUTPUT_DIRECTORY"
then
# Similarly, override this to store the test-results subdir
^ permalink raw reply related
* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Eric Sunshine @ 2023-10-03 21:10 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231003220951+0200.221551-stepnem@smrk.net>
On Tue, Oct 3, 2023 at 4:09 PM Štěpán Němec <stepnem@smrk.net> wrote:
> On Tue, 3 Oct 2023 14:30:38 -0400 Eric Sunshine wrote:
> > On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote:
> >> diff --git a/contrib/README b/contrib/README
> >> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
> >> I expect that things that start their life in the contrib/ area
> >> -to graduate out of contrib/ once they mature, either by becoming
> >> +graduate out of contrib/ once they mature, either by becoming
> >
> > You probably want to add a comma after "area".
>
> That would read awkward to me. How about going the other way,
>
> I expect things that start their life in the contrib/ area
> to graduate out of contrib/ once they mature
>
> instead?
Sounds fine.
> >> @@ -579,11 +579,10 @@ This test harness library does the following things:
> >> -Here are some recommented styles when writing test case.
> >
> > Do you want to fix the spelling error while you're here or is that
> > done in a later patch?
> >
> > s/recommented/recommended/
>
> You really had me double-checking both the branch and the patch I sent
> here. :-D Unless I'm very much missing something, that line is _removed_
> by the patch (seemed redundant given the title immediately preceding
> it).
Ugh, so it is. Sorry for the noise.
> >> - - Keep test title the same line with test helper function itself.
> >> + - Keep test titles and helper function invocations on the same line.
> >
> > This would be clearer if it was switched around. Either:
> >
> > Keep the test_expect_* function call and test title on the same line.
> >
> > or, more verbosely:
> >
> > Place the test title on the same line as the test_expect_* call
> > which precedes it.
>
> I'll take your first suggestion.
I like the first suggestion better too.
^ permalink raw reply
* Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
From: Junio C Hamano @ 2023-10-03 21:10 UTC (permalink / raw)
To: Štěpán Němec; +Cc: Eric Sunshine, git
In-Reply-To: <20231003220951+0200.221551-stepnem@smrk.net>
Štěpán Němec <stepnem@smrk.net> writes:
> Come to think of it, 'doc: fix some typos, grammar and wording issues'
> might have made sense to begin with; I don't suppose C header comments
> are off-limits to doc:.
Yup, they do count as part of developer documentation.
>>> diff --git a/contrib/README b/contrib/README
>>> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the
>>> I expect that things that start their life in the contrib/ area
>>> -to graduate out of contrib/ once they mature, either by becoming
>>> +graduate out of contrib/ once they mature, either by becoming
>>
>> You probably want to add a comma after "area".
>
> That would read awkward to me. How about going the other way,
>
> I expect things that start their life in the contrib/ area
> to graduate out of contrib/ once they mature
>
> instead?
That reads well. I do not recall whom "I" in this sentence refers
to, but if this were me talking about my wish, then yes, I expect
them to graduate once they mature, and the ">>>" quoted change to
drop "to" does not sound grammatical.
Thanks for working on improving the documentation, thanks for
reviewing, and thanks for working well together.
^ permalink raw reply
* Re: [PATCH 3/5] git-jump: admit to passing merge mode args to ls-files
From: Junio C Hamano @ 2023-10-03 21:01 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Štěpán Němec, git
In-Reply-To: <xmqqjzs3xwm1.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Unlike `--stdout` and `jump.grepCmd`, all other things that are not
> quoted, including the one that is added by this patch (i.e.,
> "ls-files -u"), are something the end-user needs to cut and paste in
"are" -> "are NOT". Sorry for the noise.
> reaction to seeing this error message, so as long as they are
> understandable in the sentences they appear in, I think they are
> fine.
>
> If we wanted to standardize, we may start to encourage consistent
> use of quoting, but I do not think it should be part of this topic.
>
> Thanks for being careful and thoughtful.
^ permalink raw reply
* [PATCH] decorate: add color.decorate.symbols config option
From: Andy Koppe @ 2023-10-03 20:54 UTC (permalink / raw)
To: git; +Cc: derrickstolee, Andy Koppe
Add new 'color.decorate.symbols' config option for determining the color
of the prefix, suffix, separator and arrow symbols used in --decorate
output and related log format placeholders, to allow them to be colored
differently from commit hashes.
For backward compatibility, fall back to the commit hash color that can
be specified with the 'color.diff.commit' option if the new option is
not provided.
Add the setting to the color.decorate.<slot> documentation, rewording
that a bit to try to improve readability.
Amend t4207-log-decoration-colors.sh to test the option.
Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
Documentation/config/color.txt | 7 ++--
commit.h | 1 +
log-tree.c | 15 ++++++---
t/t4207-log-decoration-colors.sh | 58 +++++++++++++++++---------------
4 files changed, 45 insertions(+), 36 deletions(-)
diff --git a/Documentation/config/color.txt b/Documentation/config/color.txt
index 1795b2d16b..5d3612ff09 100644
--- a/Documentation/config/color.txt
+++ b/Documentation/config/color.txt
@@ -75,9 +75,10 @@ color.diff.<slot>::
color.decorate.<slot>::
Use customized color for 'git log --decorate' output. `<slot>` is one
- of `branch`, `remoteBranch`, `tag`, `stash` or `HEAD` for local
- branches, remote-tracking branches, tags, stash and HEAD, respectively
- and `grafted` for grafted commits.
+ of `HEAD` for the current HEAD ref, `branch` for local branches,
+ `remoteBranch` for remote-tracking branches, `tag` for tags, `stash`
+ for the top stash entry, `grafted` for grafted commits, and `symbols`
+ for the punctuation surrounding the other elements.
color.grep::
When set to `always`, always highlight matches. When `false` (or
diff --git a/commit.h b/commit.h
index 28928833c5..cefcb7c490 100644
--- a/commit.h
+++ b/commit.h
@@ -56,6 +56,7 @@ enum decoration_type {
DECORATION_REF_STASH,
DECORATION_REF_HEAD,
DECORATION_GRAFTED,
+ DECORATION_SYMBOLS,
};
void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
diff --git a/log-tree.c b/log-tree.c
index 504da6b519..a5dd4292fc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -41,6 +41,7 @@ static char decoration_colors[][COLOR_MAXLEN] = {
GIT_COLOR_BOLD_MAGENTA, /* REF_STASH */
GIT_COLOR_BOLD_CYAN, /* REF_HEAD */
GIT_COLOR_BOLD_BLUE, /* GRAFTED */
+ GIT_COLOR_NIL, /* SYMBOLS */
};
static const char *color_decorate_slots[] = {
@@ -50,6 +51,7 @@ static const char *color_decorate_slots[] = {
[DECORATION_REF_STASH] = "stash",
[DECORATION_REF_HEAD] = "HEAD",
[DECORATION_GRAFTED] = "grafted",
+ [DECORATION_SYMBOLS] = "symbols",
};
static const char *decorate_get_color(int decorate_use_color, enum decoration_type ix)
@@ -312,7 +314,7 @@ void format_decorations(struct strbuf *sb,
{
const struct name_decoration *decoration;
const struct name_decoration *current_and_HEAD;
- const char *color_commit, *color_reset;
+ const char *color_symbols, *color_reset;
const char *prefix = " (";
const char *suffix = ")";
@@ -337,7 +339,10 @@ void format_decorations(struct strbuf *sb,
tag = opts->tag;
}
- color_commit = diff_get_color(use_color, DIFF_COMMIT);
+ color_symbols = decorate_get_color(use_color, DECORATION_SYMBOLS);
+ if (color_is_nil(color_symbols))
+ color_symbols = diff_get_color(use_color, DIFF_COMMIT);
+
color_reset = decorate_get_color(use_color, DECORATION_NONE);
current_and_HEAD = current_pointed_by_HEAD(decoration);
@@ -352,7 +357,7 @@ void format_decorations(struct strbuf *sb,
decorate_get_color(use_color, decoration->type);
if (*prefix) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbols);
strbuf_addstr(sb, prefix);
strbuf_addstr(sb, color_reset);
}
@@ -369,7 +374,7 @@ void format_decorations(struct strbuf *sb,
if (current_and_HEAD &&
decoration->type == DECORATION_REF_HEAD) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbols);
strbuf_addstr(sb, pointer);
strbuf_addstr(sb, color_reset);
strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
@@ -382,7 +387,7 @@ void format_decorations(struct strbuf *sb,
decoration = decoration->next;
}
if (*suffix) {
- strbuf_addstr(sb, color_commit);
+ strbuf_addstr(sb, color_symbols);
strbuf_addstr(sb, suffix);
strbuf_addstr(sb, color_reset);
}
diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index 21986a866d..663ae49d34 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -18,6 +18,7 @@ test_expect_success setup '
git config color.decorate.tag "reverse bold yellow" &&
git config color.decorate.stash magenta &&
git config color.decorate.grafted black &&
+ git config color.decorate.symbols white &&
git config color.decorate.HEAD cyan &&
c_reset="<RESET>" &&
@@ -29,6 +30,7 @@ test_expect_success setup '
c_stash="<MAGENTA>" &&
c_HEAD="<CYAN>" &&
c_grafted="<BLACK>" &&
+ c_symbols="<WHITE>" &&
test_commit A &&
git clone . other &&
@@ -53,17 +55,17 @@ cmp_filtered_decorations () {
# to this test since it does not contain any decoration, hence --first-parent
test_expect_success 'commit decorations colored correctly' '
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
-${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_commit}, \
-${c_reset}${c_remoteBranch}other/main${c_reset}${c_commit})${c_reset} A1
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_stash}refs/stash${c_reset}${c_commit})${c_reset} On main: Changes to A.t
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbols} -> ${c_reset}${c_branch}main${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbols})${c_reset} B
+${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_symbols}, \
+${c_reset}${c_remoteBranch}other/main${c_reset}${c_symbols})${c_reset} A1
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_stash}refs/stash${c_reset}${c_symbols})${c_reset} On main: Changes to A.t
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbols})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always --all >actual &&
@@ -78,14 +80,14 @@ test_expect_success 'test coloring with replace-objects' '
git replace HEAD~1 HEAD~2 &&
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit})${c_reset} D
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_commit}, \
-${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbols} -> ${c_reset}${c_branch}main${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_symbols})${c_reset} D
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_symbols}, \
+${c_reset}${c_grafted}replaced${c_reset}${c_symbols})${c_reset} B
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbols})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
@@ -104,15 +106,15 @@ test_expect_success 'test coloring with grafted commit' '
git replace --graft HEAD HEAD~2 &&
cat >expect <<-EOF &&
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD${c_reset}\
-${c_commit} -> ${c_reset}${c_branch}main${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit}, \
-${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \
-${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B
- ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\
-${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}${c_HEAD}HEAD${c_reset}\
+${c_symbols} -> ${c_reset}${c_branch}main${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_symbols}, \
+${c_reset}${c_grafted}replaced${c_reset}${c_symbols})${c_reset} D
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_symbols}, \
+${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_symbols})${c_reset} B
+ ${c_commit}COMMIT_ID${c_reset}${c_symbols} (${c_reset}\
+${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_symbols})${c_reset} A
EOF
git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
--
2.42.GIT
^ permalink raw reply related
* Re: [PATCH 3/5] git-jump: admit to passing merge mode args to ls-files
From: Junio C Hamano @ 2023-10-03 20:46 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Štěpán Němec, git
In-Reply-To: <CAPig+cQHAK2_LEG0-6MTF52t2D8b_SRxzB51B_4N35ujtGeb6Q@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
>> There's even an example of such usage in the README.
>>
>> Signed-off-by: Štěpán Němec <stepnem@smrk.net>
>> ---
>> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
>> @@ -9,7 +9,7 @@ The <mode> parameter is one of:
>>
>> diff: elements are diff hunks. Arguments are given to diff.
>>
>> -merge: elements are merge conflicts. Arguments are ignored.
>> +merge: elements are merge conflicts. Arguments are given to ls-files -u.
>
> Should "ls-files -u" be formatted with backticks?
>
> Arguments are passed to `git ls-files -u`.
Probably not in this case, as this is end-user visible help message
that is not formatted but given to the terminal.
The whole preimage looks like this:
usage: git jump [--stdout] <mode> [<args>]
Jump to interesting elements in an editor.
The <mode> parameter is one of:
diff: elements are diff hunks. Arguments are given to diff.
merge: elements are merge conflicts. Arguments are ignored.
grep: elements are grep hits. Arguments are given to git grep or, if
configured, to the command in `jump.grepCmd`.
ws: elements are whitespace errors. Arguments are given to diff --check.
If the optional argument `--stdout` is given, print the quickfix
lines to standard output instead of feeding it to the editor.
and it is already a mixture. "given to `git grep`" is not quoted,
neither is "given to `diff --check`" or "given to `diff`"
I think rule for help/usage messages should be that
- anything that the end-user may want to cut&paste should be left
alone to make it easier to cut,
- but at the same time the message should make it clear which part
of a sentence is what,
Clicking on `--stdout` or `jump.grepCmd` does not include the
surrounding quotes, at least in my environment, so the use of
backquotes in these places satisfy the two goals, it seems.
Unlike `--stdout` and `jump.grepCmd`, all other things that are not
quoted, including the one that is added by this patch (i.e.,
"ls-files -u"), are something the end-user needs to cut and paste in
reaction to seeing this error message, so as long as they are
understandable in the sentences they appear in, I think they are
fine.
If we wanted to standardize, we may start to encourage consistent
use of quoting, but I do not think it should be part of this topic.
Thanks for being careful and thoughtful.
^ permalink raw reply
* Re: batch-command wishlist [was: [TOPIC 02/12] Libification Goals and Progress]
From: Jeff King @ 2023-10-03 20:32 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Junio C Hamano, Taylor Blau
In-Reply-To: <20231003203124.M560967@dcvr>
On Tue, Oct 03, 2023 at 08:31:24PM +0000, Eric Wong wrote:
> Jeff King <peff@peff.net> wrote:
> > I know that you asked for a persistent process, but just for reference,
> > you can hackily access approxidate with:
> >
> > git config --type=expiry-date --default='15 days ago' does.not.exist
>
> --type is too new (trying to support 1.8.x :<). But yeah, using
> `git rev-parse --since=15.days.ago` and extracting the integers
Ah, that is probably less hacky than my suggestion anyway.
-Peff
^ permalink raw reply
* [PATCH 10/10] commit-graph: clear oidset after finishing write
From: Jeff King @ 2023-10-03 20:31 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
In graph_write() we store commits in an oidset, but never clean it up,
leaking the contents. We should clear it in the cleanup section.
The oidset comes from 6830c36077 (commit-graph.h: replace 'commit_hex'
with 'commits', 2020-04-13), but it was just replacing a string_list
that was also leaked. Curiously, we fixed the leak of some adjacent
variables in commit fa8953cb40 (builtin/commit-graph.c: extract
'read_one_commit()', 2020-05-18), but the oidset wasn't included for
some reason.
In combination with the preceding commits, this lets us mark t5324 as
leak-free.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/commit-graph.c | 1 +
t/t5324-split-commit-graph.sh | 2 ++
2 files changed, 3 insertions(+)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c88389df24..c527a8369e 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -311,6 +311,7 @@ static int graph_write(int argc, const char **argv, const char *prefix)
FREE_AND_NULL(options);
string_list_clear(&pack_indexes, 0);
strbuf_release(&buf);
+ oidset_clear(&commits);
return result;
}
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 36c4141e67..52e8a3e619 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='split commit graph'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
GIT_TEST_COMMIT_GRAPH=0
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
* Re: batch-command wishlist [was: [TOPIC 02/12] Libification Goals and Progress]
From: Eric Wong @ 2023-10-03 20:31 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Taylor Blau
In-Reply-To: <20231003201048.GD1562@coredump.intra.peff.net>
Jeff King <peff@peff.net> wrote:
> I know that you asked for a persistent process, but just for reference,
> you can hackily access approxidate with:
>
> git config --type=expiry-date --default='15 days ago' does.not.exist
--type is too new (trying to support 1.8.x :<). But yeah, using
`git rev-parse --since=15.days.ago` and extracting the integers
^ permalink raw reply
* [PATCH 09/10] commit-graph: free write-context base_graph_name during cleanup
From: Jeff King @ 2023-10-03 20:31 UTC (permalink / raw)
To: git
In-Reply-To: <20231003202504.GA7697@coredump.intra.peff.net>
Commit 6c622f9f0b (commit-graph: write commit-graph chains, 2019-06-18)
added a base_graph_name string to the write_commit_graph_context struct.
But the end-of-function cleanup forgot to free it, causing a leak.
This (presumably in combination with the preceding leak-fixes) lets us
mark t5328 as leak-free.
Signed-off-by: Jeff King <peff@peff.net>
---
commit-graph.c | 1 +
t/t5328-commit-graph-64bit-time.sh | 2 ++
2 files changed, 3 insertions(+)
diff --git a/commit-graph.c b/commit-graph.c
index 744b7eb1a3..e4d09da090 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2518,6 +2518,7 @@ int write_commit_graph(struct object_directory *odb,
cleanup:
free(ctx->graph_name);
+ free(ctx->base_graph_name);
free(ctx->commits.list);
oid_array_clear(&ctx->oids);
clear_topo_level_slab(&topo_levels);
diff --git a/t/t5328-commit-graph-64bit-time.sh b/t/t5328-commit-graph-64bit-time.sh
index e9c521c061..ca476e80a0 100755
--- a/t/t5328-commit-graph-64bit-time.sh
+++ b/t/t5328-commit-graph-64bit-time.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='commit graph with 64-bit timestamps'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
if ! test_have_prereq TIME_IS_64BIT || ! test_have_prereq TIME_T_IS_64BIT
--
2.42.0.810.gbc538a0ee6
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox