* [PATCH 2/2] fuzz: link fuzz programs with `make all` on Linux
From: Josh Steadmon @ 2024-03-05 21:12 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709673020.git.steadmon@google.com>
Since 5e47215080 (fuzz: add basic fuzz testing target., 2018-10-12), we
have compiled object files for the fuzz tests as part of the default
'make all' target. This helps prevent bit-rot in lesser-used parts of
the codebase, by making sure that incompatible changes are caught at
build time.
However, since we never linked the fuzzer executables, this did not
protect us from link-time errors. As of 8b9a42bf48 (fuzz: fix fuzz test
build rules, 2024-01-19), it's now possible to link the fuzzer
executables without using a fuzzing engine and a variety of
compiler-specific (and compiler-version-specific) flags, at least on
Linux. So let's add a platform-specific option in config.mak.uname to
link the executables as part of the default `make all` target.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
Makefile | 14 +++++++++++---
config.mak.uname | 1 +
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 4e255c81f2..f74e96d7c2 100644
--- a/Makefile
+++ b/Makefile
@@ -409,6 +409,9 @@ include shared.mak
# to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
# that implements the `fsm_os_settings__*()` routines.
#
+# Define LINK_FUZZ_PROGRAMS if you want `make all` to also build the fuzz test
+# programs in oss-fuzz/.
+#
# === Optional library: libintl ===
#
# Define NO_GETTEXT if you don't want Git output to be translated.
@@ -763,9 +766,6 @@ FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
.PHONY: fuzz-objs
fuzz-objs: $(FUZZ_OBJS)
-# Always build fuzz objects even if not testing, to prevent bit-rot.
-all:: $(FUZZ_OBJS)
-
FUZZ_PROGRAMS += $(patsubst %.o,%,$(filter-out %dummy-cmd-main.o,$(FUZZ_OBJS)))
# Empty...
@@ -2368,6 +2368,14 @@ ifndef NO_TCLTK
endif
$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
+# Build fuzz programs if possible, or at least compile the object files; even
+# without the necessary fuzzing support, this prevents bit-rot.
+ifdef LINK_FUZZ_PROGRAMS
+all:: $(FUZZ_PROGRAMS)
+else
+all:: $(FUZZ_OBJS)
+endif
+
please_set_SHELL_PATH_to_a_more_modern_shell:
@$$(:)
diff --git a/config.mak.uname b/config.mak.uname
index dacc95172d..6579c36a99 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -68,6 +68,7 @@ ifeq ($(uname_S),Linux)
ifneq ($(findstring .el7.,$(uname_R)),)
BASIC_CFLAGS += -std=c99
endif
+ LINK_FUZZ_PROGRAMS = YesPlease
endif
ifeq ($(uname_S),GNU/kFreeBSD)
HAVE_ALLOCA_H = YesPlease
--
2.44.0.278.ge034bb2e1d-goog
^ permalink raw reply related
* [PATCH 1/2] ci: also define CXX environment variable
From: Josh Steadmon @ 2024-03-05 21:11 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1709673020.git.steadmon@google.com>
In a future commit, we will build the fuzzer executables as part of the
default 'make all' target, which requires a C++ compiler. If we do not
explicitly set CXX, it defaults to g++ on GitHub CI. However, this can
lead to incorrect feature detection when CC=clang, since the
'detect-compiler' script only looks at CC. Fix the issue by always
setting CXX to match CC in our CI config.
We only plan on building fuzzers on Linux, so none of the other CI
configs need a similar adjustment.
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
.github/workflows/main.yml | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 683a2d633e..83945a3235 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -265,42 +265,54 @@ jobs:
vector:
- jobname: linux-sha256
cc: clang
+ cxx: clang++
pool: ubuntu-latest
- jobname: linux-reftable
cc: clang
+ cxx: clang++
pool: ubuntu-latest
- jobname: linux-gcc
cc: gcc
+ cxx: g++
cc_package: gcc-8
pool: ubuntu-20.04
- jobname: linux-TEST-vars
cc: gcc
+ cxx: g++
cc_package: gcc-8
pool: ubuntu-20.04
- jobname: osx-clang
cc: clang
+ cxx: clang++
pool: macos-13
- jobname: osx-reftable
cc: clang
+ cxx: clang++
pool: macos-13
- jobname: osx-gcc
cc: gcc
+ cxx: g++
cc_package: gcc-13
pool: macos-13
- jobname: linux-gcc-default
cc: gcc
+ cxx: g++
pool: ubuntu-latest
- jobname: linux-leaks
cc: gcc
+ cxx: g++
pool: ubuntu-latest
- jobname: linux-reftable-leaks
cc: gcc
+ cxx: g++
pool: ubuntu-latest
- jobname: linux-asan-ubsan
cc: clang
+ cxx: clang++
pool: ubuntu-latest
env:
CC: ${{matrix.vector.cc}}
+ CXX: ${{matrix.vector.cxx}}
CC_PACKAGE: ${{matrix.vector.cc_package}}
jobname: ${{matrix.vector.jobname}}
runs_on_pool: ${{matrix.vector.pool}}
--
2.44.0.278.ge034bb2e1d-goog
^ permalink raw reply related
* [PATCH 0/2] fuzz: build fuzzers by default on Linux
From: Josh Steadmon @ 2024-03-05 21:11 UTC (permalink / raw)
To: git
Increase our protection against fuzzer bit-rot by making sure we can
link the fuzz test executables on Linux. Patch 1 is a small CI config
improvement to fix compiler feature detection. Patch 2 is the Makefile /
config.mak.uname change to add the executables to `make all` on Linux.
Josh Steadmon (2):
ci: also define CXX environment variable
fuzz: link fuzz programs with `make all` on Linux
.github/workflows/main.yml | 12 ++++++++++++
Makefile | 14 +++++++++++---
config.mak.uname | 1 +
3 files changed, 24 insertions(+), 3 deletions(-)
base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
--
2.44.0.278.ge034bb2e1d-goog
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: John Cai @ 2024-03-05 20:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, Patrick Steinhardt
In-Reply-To: <xmqqplw9mviu.fsf@gitster.g>
Hi Junio,
On 4 Mar 2024, at 18:23, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> For reftable development, it would be handy to have a tool to provide
>> the direct value of any ref whether it be a symbolic ref or not.
>> Currently there is git-symbolic-ref, which only works for symbolic refs,
>> and git-rev-parse, which will resolve the ref. Let's add a --unresolved
>> option that will only take one ref and return whatever it points to
>> without dereferencing it.
>
> The approach may be reasonble, but the above description can use
> some improvements.
>
> * Even though the title of the patch says show-ref, the last
> sentence is a bit too far from there and it was unclear to what
> you are adding a new feature at least to me during my first read.
>
> Let's teach show-ref a `--unresolved` optionthat will ...
>
> may make it easier to follow.
>
> * "Whatever it points to without dereferencing it" implied that it
> assumes what it is asked to show can be dereferenced, which
> invites a natural question: what happens to a thing that is not
> dereferenceable in the first place? The implementation seems to
> show either symbolic-ref target (for symbolic refs) or the object
> name (for others), but let's make it easier for readers.
Yeah good point. The language could be made more precise.
>
>> Documentation/git-show-ref.txt | 8 ++++++
>> builtin/show-ref.c | 33 ++++++++++++++++--------
>> t/t1403-show-ref.sh | 47 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 77 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/git-show-ref.txt b/Documentation/git-show-ref.txt
>> index ba757470059..2f9b4de1346 100644
>> --- a/Documentation/git-show-ref.txt
>> +++ b/Documentation/git-show-ref.txt
>> @@ -16,6 +16,7 @@ SYNOPSIS
>> [--] [<ref>...]
>> 'git show-ref' --exclude-existing[=<pattern>]
>> 'git show-ref' --exists <ref>
>> +'git show-ref' --unresolved <ref>
>>
>> DESCRIPTION
>> -----------
>> @@ -76,6 +77,13 @@ OPTIONS
>> it does, 2 if it is missing, and 1 in case looking up the reference
>> failed with an error other than the reference being missing.
>>
>> +--unresolved::
>> +
>> + Prints out what the reference points to without resolving it. Returns
>> + an exit code of 0 if it does, 2 if it is missing, and 1 in case looking
>> + up the reference failed with an error other than the reference being
>> + missing.
>
> Exactly the same issue as in the proposed log message, i.e. what is
> printed for what kind of ref is not really clear.
>
>> -static int cmd_show_ref__exists(const char **refs)
>> +static int cmd_show_ref__raw(const char **refs, int show)
>> {
>> - struct strbuf unused_referent = STRBUF_INIT;
>> - struct object_id unused_oid;
>> - unsigned int unused_type;
>> + struct strbuf referent = STRBUF_INIT;
>> + struct object_id oid;
>> + unsigned int type;
>> int failure_errno = 0;
>> const char *ref;
>> int ret = 0;
>> @@ -236,7 +237,7 @@ static int cmd_show_ref__exists(const char **refs)
>> die("--exists requires exactly one reference");
>>
>> if (refs_read_raw_ref(get_main_ref_store(the_repository), ref,
>> - &unused_oid, &unused_referent, &unused_type,
>> + &oid, &referent, &type,
>> &failure_errno)) {
>> if (failure_errno == ENOENT || failure_errno == EISDIR) {
>> error(_("reference does not exist"));
>> @@ -250,8 +251,16 @@ static int cmd_show_ref__exists(const char **refs)
>> goto out;
>> }
>>
>> + if (!show)
>> + goto out;
>> +
>> + if (type & REF_ISSYMREF)
>> + printf("ref: %s\n", referent.buf);
>> + else
>> + printf("ref: %s\n", oid_to_hex(&oid));
>
> If I create a symbolic ref whose value is deadbeef....deadbeef 40-hex,
> I cannot tell from this output if it is a symbolic ref of a ref that
> stores an object whose name is that hash. Reserve the use of "ref: %s"
> to the symbolic refs (so that it will also match how the files backend
> stores them in modern Git), and use some other prefix (or no
> perfix).
>
> Actually, I am not sure if what is proposed is even a good
> interface. Given a repository with these few refs:
>
> $ git show-ref refs/heads/master
> b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master
> $ git show-ref refs/remotes/repo/HEAD
> b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD
> $ git symbolic-ref refs/remotes/repo/HEAD
> refs/remotes/repo/master
>
> I would think that the second command above shows the gap in feature
> set our current "show-ref" has. If we could do
>
> $ git show-ref --<option> refs/heads/master refs/remotes/repo/HEAD
> b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master
> ref:refs/remotes/repo/master refs/remotes/repo/HEAD
I like this option. It makes it clear that it's a symbolic ref without adding
additional output to the command.
cc'ing Patrick here for his thoughts as well since he has interest in this topic.
>
> or alternatively
>
> $ git show-ref --<option> refs/heads/master refs/remotes/repo/HEAD
> b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/heads/master
> ref:refs/remotes/repo/master b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD
>
> wouldn't it match the existing feature set better? You also do not
> have to limit yourself to single ref query per process invocation.
>
> I am not sure if you need to worry about quoting of the values of
> symbolic-ref, though. You _might_ need to move the (optional)
> symref information to the end, i.e. something like this you might
> prefer. I dunno.
>
> $ git show-ref --<option> refs/remotes/repo/HEAD
> b387623c12f3f4a376e4d35a610fd3e55d7ea907 refs/remotes/repo/HEAD refs/remotes/repo/master
>
> I do not know what the <option> should be called, either. From an
> end-user's point of view, the option tells the command to also
> report which ref the ref points at, if it were a symbolic one.
> "unresolved" may be technically acceptable name to those who know
> the underlying implementation (i.e. we tell read_raw_ref not to
> resolve when it does its thing), but I am afraid that is a bit too
> opaque implementation detail for end-users who are expected to learn
> this option.
I think something like --no-dereference that was suggested in [1] could work
since the concept of dereferencing should be familiar to the user. However, this
maybe confusing because of the existing --dereference flag that is specific to
tags...
1. https://lore.kernel.org/git/a3de2b7b-4603-4604-a4d2-938a598e312e@gmail.com/
thanks
John
^ permalink raw reply
* [PATCH v4 5/5] branch: advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-05 20:29 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709670287.git.code@khaugsbakk.name>
git-branch(1) will error out if you give it a bad ref name. But the user
might not understand why or what part of the name is illegal.
The user might know that there are some limitations based on the *loose
ref* format (filenames), but there are also further rules for
easier integration with shell-based tools, pathname expansion, and
playing well with reference name expressions.
The man page for git-check-ref-format(1) contains these rules. Let’s
advise about it since that is not a command that you just happen
upon. Also make this advise configurable since you might not want to be
reminded every time you make a little typo.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v4:
• Update refSyntax entry for consistency with the rest of the entries
v3:
• Tweak advice doc for the new entry
• Better test style
v2:
• Make the advise optional via configuration
• Propagate error properly with `die_message(…)` instead of `exit(1)`
• Flesh out commit message a bit
Documentation/config/advice.txt | 3 +++
advice.c | 1 +
advice.h | 1 +
branch.c | 8 ++++++--
builtin/branch.c | 8 ++++++--
t/t3200-branch.sh | 10 ++++++++++
6 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index dd52041bc94..06c754899c5 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -94,6 +94,9 @@ advice.*::
`pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
simultaneously.
+ refSyntax::
+ Shown when the user provides an illegal ref name, to
+ tell the user about the ref syntax documentation.
resetNoRefresh::
Shown when linkgit:git-reset[1] takes more than 2
seconds to refresh the index after reset, to tell the user
diff --git a/advice.c b/advice.c
index 6e9098ff089..550c2968908 100644
--- a/advice.c
+++ b/advice.c
@@ -68,6 +68,7 @@ static struct {
[ADVICE_PUSH_UNQUALIFIED_REF_NAME] = { "pushUnqualifiedRefName" },
[ADVICE_PUSH_UPDATE_REJECTED] = { "pushUpdateRejected" },
[ADVICE_PUSH_UPDATE_REJECTED_ALIAS] = { "pushNonFastForward" }, /* backwards compatibility */
+ [ADVICE_REF_SYNTAX] = { "refSyntax" },
[ADVICE_RESET_NO_REFRESH_WARNING] = { "resetNoRefresh" },
[ADVICE_RESOLVE_CONFLICT] = { "resolveConflict" },
[ADVICE_RM_HINTS] = { "rmHints" },
diff --git a/advice.h b/advice.h
index 9d4f49ae38b..d15fe2351ab 100644
--- a/advice.h
+++ b/advice.h
@@ -36,6 +36,7 @@ enum advice_type {
ADVICE_PUSH_UNQUALIFIED_REF_NAME,
ADVICE_PUSH_UPDATE_REJECTED,
ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
+ ADVICE_REF_SYNTAX,
ADVICE_RESET_NO_REFRESH_WARNING,
ADVICE_RESOLVE_CONFLICT,
ADVICE_RM_HINTS,
diff --git a/branch.c b/branch.c
index 6719a181bd1..621019fcf4b 100644
--- a/branch.c
+++ b/branch.c
@@ -370,8 +370,12 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
*/
int validate_branchname(const char *name, struct strbuf *ref)
{
- if (strbuf_check_branch_ref(ref, name))
- die(_("'%s' is not a valid branch name"), name);
+ if (strbuf_check_branch_ref(ref, name)) {
+ int code = die_message(_("'%s' is not a valid branch name"), name);
+ advise_if_enabled(ADVICE_REF_SYNTAX,
+ _("See `man git check-ref-format`"));
+ exit(code);
+ }
return ref_exists(ref->buf);
}
diff --git a/builtin/branch.c b/builtin/branch.c
index cfb63cce5fb..1c122ee8a7b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -576,8 +576,12 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
*/
if (ref_exists(oldref.buf))
recovery = 1;
- else
- die(_("invalid branch name: '%s'"), oldname);
+ else {
+ int code = die_message(_("invalid branch name: '%s'"), oldname);
+ advise_if_enabled(ADVICE_REF_SYNTAX,
+ _("See `man git check-ref-format`"));
+ exit(code);
+ }
}
for (int i = 0; worktrees[i]; i++) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 060b27097e8..dd7525d1b8c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1722,4 +1722,14 @@ test_expect_success '--track overrides branch.autoSetupMerge' '
test_cmp_config "" --default "" branch.foo5.merge
'
+test_expect_success 'errors if given a bad branch name' '
+ cat <<-\EOF >expect &&
+ fatal: '\''foo..bar'\'' is not a valid branch name
+ hint: See `man git check-ref-format`
+ hint: Disable this message with "git config advice.refSyntax false"
+ EOF
+ test_must_fail git branch foo..bar >actual 2>&1 &&
+ test_cmp expect actual
+'
+
test_done
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* [PATCH v4 4/5] advice: use double quotes for regular quoting
From: Kristoffer Haugsbakk @ 2024-03-05 20:29 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709670287.git.code@khaugsbakk.name>
Use double quotes like we use for “die” in this document.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Documentation/config/advice.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c8d6c625f2a..dd52041bc94 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -75,7 +75,7 @@ advice.*::
non-fast-forward update to the current branch.
pushNonFFMatching::
Shown when the user ran linkgit:git-push[1] and pushed
- 'matching refs' explicitly (i.e. used `:`, or
+ "matching refs" explicitly (i.e. used `:`, or
specified a refspec that isn't the current branch) and
it resulted in a non-fast-forward error.
pushRefNeedsUpdate::
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* [PATCH v4 3/5] advice: use backticks for verbatim
From: Kristoffer Haugsbakk @ 2024-03-05 20:29 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709670287.git.code@khaugsbakk.name>
Use backticks for inline-verbatim rather than single quotes. Also quote
the unquoted ref globs.
Also replace “the add command” with “`git add`”.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v4:
• Also quote ref globs
Documentation/config/advice.txt | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 72cd9f9e9d9..c8d6c625f2a 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -2,14 +2,14 @@ advice.*::
These variables control various optional help messages designed to
aid new users. When left unconfigured, Git will give the message
alongside instructions on how to squelch it. You can tell Git
- that you do not need the help message by setting these to 'false':
+ that you do not need the help message by setting these to `false`:
+
--
addEmbeddedRepo::
Shown when the user accidentally adds one
git repo inside of another.
addEmptyPathspec::
- Shown when the user runs the add command without providing
+ Shown when the user runs `git add` without providing
the pathspec parameter.
addIgnoredFile::
Shown when the user attempts to add an ignored file to
@@ -75,7 +75,7 @@ advice.*::
non-fast-forward update to the current branch.
pushNonFFMatching::
Shown when the user ran linkgit:git-push[1] and pushed
- 'matching refs' explicitly (i.e. used ':', or
+ 'matching refs' explicitly (i.e. used `:`, or
specified a refspec that isn't the current branch) and
it resulted in a non-fast-forward error.
pushRefNeedsUpdate::
@@ -87,12 +87,12 @@ advice.*::
guess based on the source and destination refs what
remote ref namespace the source belongs in, but where
we can still suggest that the user push to either
- refs/heads/* or refs/tags/* based on the type of the
+ `refs/heads/*` or `refs/tags/*` based on the type of the
source object.
pushUpdateRejected::
- Set this variable to 'false' if you want to disable
- 'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
- 'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
+ Set this variable to `false` if you want to disable
+ `pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
+ `pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
simultaneously.
resetNoRefresh::
Shown when linkgit:git-reset[1] takes more than 2
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* [PATCH v4 2/5] advice: make all entries stylistically consistent
From: Kristoffer Haugsbakk @ 2024-03-05 20:29 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709670287.git.code@khaugsbakk.name>
In general, rewrite entries to the following form:
1. Clause or sentence describing when the advice is shown
2. Optional “to <verb>” clause which says what the advice is
about (e.g. for resetNoRefresh: tell the user that they can use
`--no-refresh`)
Concretely:
1. Use “shown” instead of “advice shown”
• “advice” is implied and a bit repetitive
2. Use “when” instead of “if”
3. Lead with “Shown when” and end the entry with the effect it has,
where applicable
4. Use “the user” instead of “a user” or “you”
5. implicitIdentity: rewrite description in order to lead with *when*
the advice is shown (see point (3))
6. Prefer the present tense (with the exception of pushNonFFMatching)
7. waitingForEditor: give example of relevance in this new context
8. pushUpdateRejected: exception to the above principles
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v4:
• Drop trailer since this took on a life of its own
• Drop uses of colons and semicolons in favor of a “to <verb>”
clause (mostly “to tell”)
• Simplify some of the “effect clauses” by using “to tell” instead of
verbs like “instruct”
v3:
• Comment: Maybe the style that we eventually agree on should be
documented outside the commit log?
Documentation/config/advice.txt | 82 ++++++++++++++++-----------------
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..72cd9f9e9d9 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -6,23 +6,23 @@ advice.*::
+
--
addEmbeddedRepo::
- Advice on what to do when you've accidentally added one
+ Shown when the user accidentally adds one
git repo inside of another.
addEmptyPathspec::
- Advice shown if a user runs the add command without providing
+ Shown when the user runs the add command without providing
the pathspec parameter.
addIgnoredFile::
- Advice shown if a user attempts to add an ignored file to
+ Shown when the user attempts to add an ignored file to
the index.
amWorkDir::
- Advice that shows the location of the patch file when
- linkgit:git-am[1] fails to apply it.
+ Shown when linkgit:git-am[1] fails to apply a patch
+ file, to tell the user the location of the file.
ambiguousFetchRefspec::
- Advice shown when a fetch refspec for multiple remotes maps to
+ Shown when a fetch refspec for multiple remotes maps to
the same remote-tracking branch namespace and causes branch
tracking set-up to fail.
checkoutAmbiguousRemoteBranchName::
- Advice shown when the argument to
+ Shown when the argument to
linkgit:git-checkout[1] and linkgit:git-switch[1]
ambiguously resolves to a
remote tracking branch on more than one remote in
@@ -33,31 +33,31 @@ advice.*::
to be used by default in some situations where this
advice would be printed.
commitBeforeMerge::
- Advice shown when linkgit:git-merge[1] refuses to
+ Shown when linkgit:git-merge[1] refuses to
merge to avoid overwriting local changes.
detachedHead::
- Advice shown when you used
+ Shown when the user uses
linkgit:git-switch[1] or linkgit:git-checkout[1]
- to move to the detached HEAD state, to instruct how to
- create a local branch after the fact.
+ to move to the detached HEAD state, to tell the user how
+ to create a local branch after the fact.
diverging::
- Advice shown when a fast-forward is not possible.
+ Shown when a fast-forward is not possible.
fetchShowForcedUpdates::
- Advice shown when linkgit:git-fetch[1] takes a long time
+ Shown when linkgit:git-fetch[1] takes a long time
to calculate forced updates after ref updates, or to warn
that the check is disabled.
forceDeleteBranch::
- Advice shown when a user tries to delete a not fully merged
+ Shown when the user tries to delete a not fully merged
branch without the force option set.
ignoredHook::
- Advice shown if a hook is ignored because the hook is not
+ Shown when a hook is ignored because the hook is not
set as executable.
implicitIdentity::
- Advice on how to set your identity configuration when
- your information is guessed from the system username and
- domain name.
+ Shown when the user's information is guessed from the
+ system username and domain name, to tell the user how to
+ set their identity configuration.
nestedTag::
- Advice shown if a user attempts to recursively tag a tag object.
+ Shown when a user attempts to recursively tag a tag object.
pushAlreadyExists::
Shown when linkgit:git-push[1] rejects an update that
does not qualify for fast-forwarding (e.g., a tag.)
@@ -71,12 +71,12 @@ advice.*::
object that is not a commit-ish, or make the remote
ref point at an object that is not a commit-ish.
pushNonFFCurrent::
- Advice shown when linkgit:git-push[1] fails due to a
+ Shown when linkgit:git-push[1] fails due to a
non-fast-forward update to the current branch.
pushNonFFMatching::
- Advice shown when you ran linkgit:git-push[1] and pushed
- 'matching refs' explicitly (i.e. you used ':', or
- specified a refspec that isn't your current branch) and
+ Shown when the user ran linkgit:git-push[1] and pushed
+ 'matching refs' explicitly (i.e. used ':', or
+ specified a refspec that isn't the current branch) and
it resulted in a non-fast-forward error.
pushRefNeedsUpdate::
Shown when linkgit:git-push[1] rejects a forced update of
@@ -95,17 +95,17 @@ advice.*::
'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
simultaneously.
resetNoRefresh::
- Advice to consider using the `--no-refresh` option to
- linkgit:git-reset[1] when the command takes more than 2 seconds
- to refresh the index after reset.
+ Shown when linkgit:git-reset[1] takes more than 2
+ seconds to refresh the index after reset, to tell the user
+ that they can use the `--no-refresh` option.
resolveConflict::
- Advice shown by various commands when conflicts
+ Shown by various commands when conflicts
prevent the operation from being performed.
rmHints::
- In case of failure in the output of linkgit:git-rm[1],
- show directions on how to proceed from the current state.
+ Shown on failure in the output of linkgit:git-rm[1], to
+ give directions on how to proceed from the current state.
sequencerInUse::
- Advice shown when a sequencer command is already in progress.
+ Shown when a sequencer command is already in progress.
skippedCherryPicks::
Shown when linkgit:git-rebase[1] skips a commit that has already
been cherry-picked onto the upstream branch.
@@ -123,27 +123,27 @@ advice.*::
by linkgit:git-switch[1] or
linkgit:git-checkout[1] when switching branches.
statusUoption::
- Advise to consider using the `-u` option to linkgit:git-status[1]
- when the command takes more than 2 seconds to enumerate untracked
- files.
+ Shown when linkgit:git-status[1] takes more than 2
+ seconds to enumerate untracked files, to tell the user that
+ they can use the `-u` option.
submoduleAlternateErrorStrategyDie::
- Advice shown when a submodule.alternateErrorStrategy option
+ Shown when a submodule.alternateErrorStrategy option
configured to "die" causes a fatal error.
submodulesNotUpdated::
- Advice shown when a user runs a submodule command that fails
+ Shown when a user runs a submodule command that fails
because `git submodule update --init` was not run.
suggestDetachingHead::
- Advice shown when linkgit:git-switch[1] refuses to detach HEAD
+ Shown when linkgit:git-switch[1] refuses to detach HEAD
without the explicit `--detach` option.
updateSparsePath::
- Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1]
+ Shown when either linkgit:git-add[1] or linkgit:git-rm[1]
is asked to update index entries outside the current sparse
checkout.
waitingForEditor::
- Print a message to the terminal whenever Git is waiting for
- editor input from the user.
+ Shown when Git is waiting for editor input. Relevant
+ when e.g. the editor is not launched inside the terminal.
worktreeAddOrphan::
- Advice shown when a user tries to create a worktree from an
- invalid reference, to instruct how to create a new unborn
+ Shown when the user tries to create a worktree from an
+ invalid reference, to tell the user how to create a new unborn
branch instead.
--
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* [PATCH v4 1/5] t3200: improve test style
From: Kristoffer Haugsbakk @ 2024-03-05 20:29 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Junio C Hamano
In-Reply-To: <cover.1709670287.git.code@khaugsbakk.name>
Some tests use a preliminary heredoc for `expect` or have setup and
teardown commands before and after, respectively. It is however
preferred to keep all the logic in the test itself. Let’s move these
into the tests.
Also:
• Remove a now-irrelevant comment about test placement and switch back
to `main` post-test
• Prefer indented literal heredocs (`-\EOF`) except for a block which
says that this is intentional
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v4:
• Drop `(setup)` change
• Drop superflouos bullet point
• Don’t use period to end bullet point
t/t3200-branch.sh | 113 ++++++++++++++++++++++------------------------
1 file changed, 55 insertions(+), 58 deletions(-)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4f..060b27097e8 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -75,13 +75,13 @@ test_expect_success 'git branch HEAD should fail' '
test_must_fail git branch HEAD
'
-cat >expect <<EOF
-$HEAD refs/heads/d/e/f@{0}: branch: Created from main
-EOF
test_expect_success 'git branch --create-reflog d/e/f should create a branch and a log' '
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git -c core.logallrefupdates=false branch --create-reflog d/e/f &&
test_ref_exists refs/heads/d/e/f &&
+ cat >expect <<-EOF &&
+ $HEAD refs/heads/d/e/f@{0}: branch: Created from main
+ EOF
git reflog show --no-abbrev-commit refs/heads/d/e/f >actual &&
test_cmp expect actual
'
@@ -440,10 +440,10 @@ test_expect_success 'git branch --list -v with --abbrev' '
test_expect_success 'git branch --column' '
COLUMNS=81 git branch --column=column >actual &&
- cat >expect <<\EOF &&
- a/b/c bam foo l * main n o/p r
- abc bar j/k m/m mb o/o q topic
-EOF
+ cat >expect <<-\EOF &&
+ a/b/c bam foo l * main n o/p r
+ abc bar j/k m/m mb o/o q topic
+ EOF
test_cmp expect actual
'
@@ -453,25 +453,25 @@ test_expect_success 'git branch --column with an extremely long branch name' '
test_when_finished "git branch -d $long" &&
git branch $long &&
COLUMNS=80 git branch --column=column >actual &&
- cat >expect <<EOF &&
- a/b/c
- abc
- bam
- bar
- foo
- j/k
- l
- m/m
-* main
- mb
- n
- o/o
- o/p
- q
- r
- topic
- $long
-EOF
+ cat >expect <<-EOF &&
+ a/b/c
+ abc
+ bam
+ bar
+ foo
+ j/k
+ l
+ m/m
+ * main
+ mb
+ n
+ o/o
+ o/p
+ q
+ r
+ topic
+ $long
+ EOF
test_cmp expect actual
'
@@ -481,10 +481,10 @@ test_expect_success 'git branch with column.*' '
COLUMNS=80 git branch >actual &&
git config --unset column.branch &&
git config --unset column.ui &&
- cat >expect <<\EOF &&
- a/b/c bam foo l * main n o/p r
- abc bar j/k m/m mb o/o q topic
-EOF
+ cat >expect <<-\EOF &&
+ a/b/c bam foo l * main n o/p r
+ abc bar j/k m/m mb o/o q topic
+ EOF
test_cmp expect actual
'
@@ -496,39 +496,36 @@ test_expect_success 'git branch -v with column.ui ignored' '
git config column.ui column &&
COLUMNS=80 git branch -v | cut -c -8 | sed "s/ *$//" >actual &&
git config --unset column.ui &&
- cat >expect <<\EOF &&
- a/b/c
- abc
- bam
- bar
- foo
- j/k
- l
- m/m
-* main
- mb
- n
- o/o
- o/p
- q
- r
- topic
-EOF
+ cat >expect <<-\EOF &&
+ a/b/c
+ abc
+ bam
+ bar
+ foo
+ j/k
+ l
+ m/m
+ * main
+ mb
+ n
+ o/o
+ o/p
+ q
+ r
+ topic
+ EOF
test_cmp expect actual
'
-mv .git/config .git/config-saved
-
test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
+ test_when_finished mv .git/config-saved .git/config &&
+ mv .git/config .git/config-saved &&
git branch -m q q2 &&
git branch -m q2 q
'
-mv .git/config-saved .git/config
-
-git config branch.s/s.dummy Hello
-
test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
+ git config branch.s/s.dummy Hello &&
git branch --create-reflog s/s &&
git reflog exists refs/heads/s/s &&
git branch --create-reflog s/t &&
@@ -1141,14 +1138,14 @@ test_expect_success '--set-upstream-to notices an error to set branch as own ups
test_cmp expect actual
"
-# Keep this test last, as it changes the current branch
-cat >expect <<EOF
-$HEAD refs/heads/g/h/i@{0}: branch: Created from main
-EOF
test_expect_success 'git checkout -b g/h/i -l should create a branch and a log' '
+ test_when_finished git checkout main &&
GIT_COMMITTER_DATE="2005-05-26 23:30" \
git checkout -b g/h/i -l main &&
test_ref_exists refs/heads/g/h/i &&
+ cat >expect <<-EOF &&
+ $HEAD refs/heads/g/h/i@{0}: branch: Created from main
+ EOF
git reflog show --no-abbrev-commit refs/heads/g/h/i >actual &&
test_cmp expect actual
'
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* [PATCH v4 0/5] advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-05 20:29 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Elijah Newren, Jean-Noël Avila
In-Reply-To: <cover.1709590037.git.code@khaugsbakk.name>
Point the user towards the ref/branch name syntax rules if they give an
invalid name.
Also make some spatially-appropriate improvements:
• Test style
• `advice.txt`
§ git-replace(1)
(see cover letter for v2)
§ Alternatives (to this change)
While working on this I also thought that it might be nice to have a
man page `gitrefsyntax`. That one could use a lot of the content from
`man git check-ref-format` verbatim. Then the hint could point towards
that man page. And it seems that AsciiDoc supports _includes_ which
means that the rules don’t have to be duplicated between the two man
pages.
§ CC
For changes to `advice.txt`:
Cc: Elijah Newren <newren@gmail.com>
Cc: Jean-Noël Avila <avila.jn@gmail.com>
§ Changes in v4
Mostly about the style rewrite in `advice.txt`.
• Patch 1:
• Drop `(setup)` change
• Drop superflouos bullet point
• Don’t use period to end bullet point
• Patch 2:
• Drop trailer since this took on a life of its own
• Drop uses of colons and semicolons in favor of a “to <verb>”
clause (mostly “to tell”)
• Simplify some of the “effect clauses” by using “to tell” instead of
verbs like “instruct”
• Patch 3:
• Also quote ref globs
• Patch 5:
• Update refSyntax entry for consistency with the rest of the entries
Kristoffer Haugsbakk (5):
t3200: improve test style
advice: make all entries stylistically consistent
advice: use backticks for verbatim
advice: use double quotes for regular quoting
branch: advise about ref syntax rules
Documentation/config/advice.txt | 95 ++++++++++++------------
advice.c | 1 +
advice.h | 1 +
branch.c | 8 ++-
builtin/branch.c | 8 ++-
t/t3200-branch.sh | 123 +++++++++++++++++---------------
6 files changed, 128 insertions(+), 108 deletions(-)
Range-diff against v3:
1: e6a2628ce57 ! 1: ad101c72a60 t3200: improve test style
@@ Commit message
Also:
• Remove a now-irrelevant comment about test placement and switch back
- to `main` post-test.
+ to `main` post-test
• Prefer indented literal heredocs (`-\EOF`) except for a block which
says that this is intentional
- • Move a `git config` command into the test and mark it as `setup` since
- the next test depends on it
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
+
+ ## Notes (series) ##
+ v4:
+ • Drop `(setup)` change
+ • Drop superflouos bullet point
+ • Don’t use period to end bullet point
+
## t/t3200-branch.sh ##
@@ t/t3200-branch.sh: test_expect_success 'git branch HEAD should fail' '
test_must_fail git branch HEAD
@@ t/t3200-branch.sh: test_expect_success 'git branch -v with column.ui ignored' '
-
-git config branch.s/s.dummy Hello
-
--test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
-+test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' '
+ test_expect_success 'git branch -m s/s s should work when s/t is deleted' '
+ git config branch.s/s.dummy Hello &&
git branch --create-reflog s/s &&
git reflog exists refs/heads/s/s &&
2: d48b4719c27 ! 2: 7017ff3fff7 advice: make all entries stylistically consistent
@@ Metadata
## Commit message ##
advice: make all entries stylistically consistent
+ In general, rewrite entries to the following form:
+
+ 1. Clause or sentence describing when the advice is shown
+ 2. Optional “to <verb>” clause which says what the advice is
+ about (e.g. for resetNoRefresh: tell the user that they can use
+ `--no-refresh`)
+
+ Concretely:
+
1. Use “shown” instead of “advice shown”
• “advice” is implied and a bit repetitive
2. Use “when” instead of “if”
3. Lead with “Shown when” and end the entry with the effect it has,
where applicable
4. Use “the user” instead of “a user” or “you”
- 5. detachedHead: connect clause with a semicolon to make the sentence
- flow better in this new context
- 6. implicitIdentity: rewrite description in order to lead with *when*
+ 5. implicitIdentity: rewrite description in order to lead with *when*
the advice is shown (see point (3))
- 7. Prefer the present tense (with the exception of pushNonFFMatching)
- 8. Use a colon to connect the last clause instead of a comma
- 9. waitingForEditor: give example of relevance in this new context
- 10. pushUpdateRejected: exception to the above principles
+ 6. Prefer the present tense (with the exception of pushNonFFMatching)
+ 7. waitingForEditor: give example of relevance in this new context
+ 8. pushUpdateRejected: exception to the above principles
- Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Notes (series) ##
- Maybe the style that we eventually agree on should be documented outside the
- commit log?
+ v4:
+ • Drop trailer since this took on a life of its own
+ • Drop uses of colons and semicolons in favor of a “to <verb>”
+ clause (mostly “to tell”)
+ • Simplify some of the “effect clauses” by using “to tell” instead of
+ verbs like “instruct”
+ v3:
+ • Comment: Maybe the style that we eventually agree on should be
+ documented outside the commit log?
## Documentation/config/advice.txt ##
@@ Documentation/config/advice.txt: advice.*::
@@ Documentation/config/advice.txt: advice.*::
- Advice that shows the location of the patch file when
- linkgit:git-am[1] fails to apply it.
+ Shown when linkgit:git-am[1] fails to apply a patch
-+ file: tell the location of the file.
++ file, to tell the user the location of the file.
ambiguousFetchRefspec::
- Advice shown when a fetch refspec for multiple remotes maps to
+ Shown when a fetch refspec for multiple remotes maps to
@@ Documentation/config/advice.txt: advice.*::
+ Shown when the user uses
linkgit:git-switch[1] or linkgit:git-checkout[1]
- to move to the detached HEAD state, to instruct how to
-+ to move to the detached HEAD state; instruct how to
- create a local branch after the fact.
+- create a local branch after the fact.
++ to move to the detached HEAD state, to tell the user how
++ to create a local branch after the fact.
diverging::
- Advice shown when a fast-forward is not possible.
+ Shown when a fast-forward is not possible.
@@ Documentation/config/advice.txt: advice.*::
- your information is guessed from the system username and
- domain name.
+ Shown when the user's information is guessed from the
-+ system username and domain name: tell the user how to
++ system username and domain name, to tell the user how to
+ set their identity configuration.
nestedTag::
- Advice shown if a user attempts to recursively tag a tag object.
@@ Documentation/config/advice.txt: advice.*::
- linkgit:git-reset[1] when the command takes more than 2 seconds
- to refresh the index after reset.
+ Shown when linkgit:git-reset[1] takes more than 2
-+ seconds to refresh the index after reset: tell the user
++ seconds to refresh the index after reset, to tell the user
+ that they can use the `--no-refresh` option.
resolveConflict::
- Advice shown by various commands when conflicts
@@ Documentation/config/advice.txt: advice.*::
rmHints::
- In case of failure in the output of linkgit:git-rm[1],
- show directions on how to proceed from the current state.
-+ Shown on failure in the output of linkgit:git-rm[1]:
++ Shown on failure in the output of linkgit:git-rm[1], to
+ give directions on how to proceed from the current state.
sequencerInUse::
- Advice shown when a sequencer command is already in progress.
@@ Documentation/config/advice.txt: advice.*::
- when the command takes more than 2 seconds to enumerate untracked
- files.
+ Shown when linkgit:git-status[1] takes more than 2
-+ seconds to enumerate untracked files: consider using the
-+ `-u` option.
++ seconds to enumerate untracked files, to tell the user that
++ they can use the `-u` option.
submoduleAlternateErrorStrategyDie::
- Advice shown when a submodule.alternateErrorStrategy option
+ Shown when a submodule.alternateErrorStrategy option
@@ Documentation/config/advice.txt: advice.*::
- Advice shown when a user tries to create a worktree from an
- invalid reference, to instruct how to create a new unborn
+ Shown when the user tries to create a worktree from an
-+ invalid reference: instruct how to create a new unborn
++ invalid reference, to tell the user how to create a new unborn
branch instead.
--
3: 30d662a04c7 ! 3: df9b872afd1 advice: use backticks for code
@@ Metadata
Author: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Commit message ##
- advice: use backticks for code
+ advice: use backticks for verbatim
- Use backticks for quoting code rather than single quotes.
+ Use backticks for inline-verbatim rather than single quotes. Also quote
+ the unquoted ref globs.
Also replace “the add command” with “`git add`”.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
+
+ ## Notes (series) ##
+ v4:
+ • Also quote ref globs
+
## Documentation/config/advice.txt ##
@@ Documentation/config/advice.txt: advice.*::
These variables control various optional help messages designed to
@@ Documentation/config/advice.txt: advice.*::
it resulted in a non-fast-forward error.
pushRefNeedsUpdate::
@@ Documentation/config/advice.txt: advice.*::
- refs/heads/* or refs/tags/* based on the type of the
+ guess based on the source and destination refs what
+ remote ref namespace the source belongs in, but where
+ we can still suggest that the user push to either
+- refs/heads/* or refs/tags/* based on the type of the
++ `refs/heads/*` or `refs/tags/*` based on the type of the
source object.
pushUpdateRejected::
- Set this variable to 'false' if you want to disable
4: 3028713357f = 4: 15594b2a3a8 advice: use double quotes for regular quoting
5: 402b7937951 ! 5: 97b53c04894 branch: advise about ref syntax rules
@@ Commit message
## Notes (series) ##
+ v4:
+ • Update refSyntax entry for consistency with the rest of the entries
v3:
• Tweak advice doc for the new entry
• Better test style
@@ Documentation/config/advice.txt: advice.*::
`pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
simultaneously.
+ refSyntax::
-+ Shown when the user provides an illegal ref name: point
-+ towards the ref syntax documentation.
++ Shown when the user provides an illegal ref name, to
++ tell the user about the ref syntax documentation.
resetNoRefresh::
Shown when linkgit:git-reset[1] takes more than 2
- seconds to refresh the index after reset: tell the user
+ seconds to refresh the index after reset, to tell the user
## advice.c ##
@@ advice.c: static struct {
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply
* [PATCH] reftable/stack: use geometric table compaction
From: Justin Tobler via GitGitGadget @ 2024-03-05 20:03 UTC (permalink / raw)
To: git; +Cc: Justin Tobler, Justin Tobler
From: Justin Tobler <jltobler@gmail.com>
To reduce the number of on-disk reftables, compaction is performed.
Contiguous tables with the same binary log value of size are grouped
into segments. The segment that has both the lowest binary log value and
contains more than one table is set as the starting point when
identifying the compaction segment.
Since segments containing a single table are not initially considered
for compaction, if the table appended to the list does not match the
previous table log value, no compaction occurs for the new table. It is
therefore possible for unbounded growth of the table list. This can be
demonstrated by repeating the following sequence:
git branch -f foo
git branch -d foo
Each operation results in a new table being written with no compaction
occurring until a separate operation produces a table matching the
previous table log value.
To avoid unbounded growth of the table list, walk through each table and
evaluate if it needs to be included in the compaction segment to restore
a geometric sequence.
Some tests in `t0610-reftable-basics.sh` assert the on-disk state of
tables and are therefore updated to specify the correct new table count.
Since compaction is more aggressive in ensuring tables maintain a
geometric sequence, the expected table count is reduced in these tests.
In `reftable/stack_test.c` tests related to `sizes_to_segments()` are
removed because the function is no longer needed. Also, the
`test_suggest_compaction_segment()` test is updated to better showcase
and reflect the new geometric compaction behavior.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
---
reftable/stack: use geometric table compaction
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1683%2Fjltobler%2Fjt%2Freftable-geometric-compaction-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1683/jltobler/jt/reftable-geometric-compaction-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1683
reftable/stack.c | 106 +++++++++++++++----------------------
reftable/stack.h | 3 --
reftable/stack_test.c | 66 +++++------------------
t/t0610-reftable-basics.sh | 24 ++++-----
4 files changed, 70 insertions(+), 129 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index b64e55648aa..e4ea8753977 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -1214,75 +1214,57 @@ static int segment_size(struct segment *s)
return s->end - s->start;
}
-int fastlog2(uint64_t sz)
-{
- int l = 0;
- if (sz == 0)
- return 0;
- for (; sz; sz /= 2) {
- l++;
- }
- return l - 1;
-}
-
-struct segment *sizes_to_segments(size_t *seglen, uint64_t *sizes, size_t n)
-{
- struct segment *segs = reftable_calloc(n, sizeof(*segs));
- struct segment cur = { 0 };
- size_t next = 0, i;
-
- if (n == 0) {
- *seglen = 0;
- return segs;
- }
- for (i = 0; i < n; i++) {
- int log = fastlog2(sizes[i]);
- if (cur.log != log && cur.bytes > 0) {
- struct segment fresh = {
- .start = i,
- };
-
- segs[next++] = cur;
- cur = fresh;
- }
-
- cur.log = log;
- cur.end = i + 1;
- cur.bytes += sizes[i];
- }
- segs[next++] = cur;
- *seglen = next;
- return segs;
-}
-
struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
{
- struct segment min_seg = {
- .log = 64,
- };
- struct segment *segs;
- size_t seglen = 0, i;
-
- segs = sizes_to_segments(&seglen, sizes, n);
- for (i = 0; i < seglen; i++) {
- if (segment_size(&segs[i]) == 1)
- continue;
+ struct segment seg = { 0 };
+ uint64_t bytes;
+ size_t i;
- if (segs[i].log < min_seg.log)
- min_seg = segs[i];
- }
+ /*
+ * If there are no tables or only a single one then we don't have to
+ * compact anything. The sequence is geometric by definition already.
+ */
+ if (n <= 1)
+ return seg;
- while (min_seg.start > 0) {
- size_t prev = min_seg.start - 1;
- if (fastlog2(min_seg.bytes) < fastlog2(sizes[prev]))
+ /*
+ * Find the ending table of the compaction segment needed to restore the
+ * geometric sequence.
+ *
+ * To do so, we iterate backwards starting from the most recent table
+ * until a valid segment end is found. If the preceding table is smaller
+ * than the current table multiplied by the geometric factor (2), the
+ * current table is set as the compaction segment end.
+ */
+ for (i = n - 1; i > 0; i--) {
+ if (sizes[i - 1] < sizes[i] * 2) {
+ seg.end = i;
+ bytes = sizes[i];
break;
+ }
+ }
+
+ /*
+ * Find the starting table of the compaction segment by iterating
+ * through the remaing tables and keeping track of the accumulated size
+ * of all tables seen from the segment end table.
+ *
+ * Note that we keep iterating even after we have found the first
+ * first starting point. This is because there may be tables in the
+ * stack preceding that first starting point which violate the geometric
+ * sequence.
+ */
+ for (; i > 0; i--) {
+ uint64_t curr = bytes;
+ bytes += sizes[i - 1];
- min_seg.start = prev;
- min_seg.bytes += sizes[prev];
+ if (sizes[i - 1] < curr * 2) {
+ seg.start = i - 1;
+ seg.bytes = bytes;
+ }
}
- reftable_free(segs);
- return min_seg;
+ return seg;
}
static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
@@ -1305,7 +1287,7 @@ int reftable_stack_auto_compact(struct reftable_stack *st)
suggest_compaction_segment(sizes, st->merged->stack_len);
reftable_free(sizes);
if (segment_size(&seg) > 0)
- return stack_compact_range_stats(st, seg.start, seg.end - 1,
+ return stack_compact_range_stats(st, seg.start, seg.end,
NULL);
return 0;
diff --git a/reftable/stack.h b/reftable/stack.h
index d919455669e..656f896cc28 100644
--- a/reftable/stack.h
+++ b/reftable/stack.h
@@ -33,12 +33,9 @@ int read_lines(const char *filename, char ***lines);
struct segment {
size_t start, end;
- int log;
uint64_t bytes;
};
-int fastlog2(uint64_t sz);
-struct segment *sizes_to_segments(size_t *seglen, uint64_t *sizes, size_t n);
struct segment suggest_compaction_segment(uint64_t *sizes, size_t n);
#endif
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 509f4866236..85600a9573e 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -720,59 +720,14 @@ static void test_reftable_stack_hash_id(void)
clear_dir(dir);
}
-static void test_log2(void)
-{
- EXPECT(1 == fastlog2(3));
- EXPECT(2 == fastlog2(4));
- EXPECT(2 == fastlog2(5));
-}
-
-static void test_sizes_to_segments(void)
-{
- uint64_t sizes[] = { 2, 3, 4, 5, 7, 9 };
- /* .................0 1 2 3 4 5 */
-
- size_t seglen = 0;
- struct segment *segs =
- sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));
- EXPECT(segs[2].log == 3);
- EXPECT(segs[2].start == 5);
- EXPECT(segs[2].end == 6);
-
- EXPECT(segs[1].log == 2);
- EXPECT(segs[1].start == 2);
- EXPECT(segs[1].end == 5);
- reftable_free(segs);
-}
-
-static void test_sizes_to_segments_empty(void)
-{
- size_t seglen = 0;
- struct segment *segs = sizes_to_segments(&seglen, NULL, 0);
- EXPECT(seglen == 0);
- reftable_free(segs);
-}
-
-static void test_sizes_to_segments_all_equal(void)
-{
- uint64_t sizes[] = { 5, 5 };
- size_t seglen = 0;
- struct segment *segs =
- sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));
- EXPECT(seglen == 1);
- EXPECT(segs[0].start == 0);
- EXPECT(segs[0].end == 2);
- reftable_free(segs);
-}
-
static void test_suggest_compaction_segment(void)
{
- uint64_t sizes[] = { 128, 64, 17, 16, 9, 9, 9, 16, 16 };
+ uint64_t sizes[] = { 512, 64, 17, 16, 9, 9, 9, 16, 2, 16 };
/* .................0 1 2 3 4 5 6 */
struct segment min =
suggest_compaction_segment(sizes, ARRAY_SIZE(sizes));
- EXPECT(min.start == 2);
- EXPECT(min.end == 7);
+ EXPECT(min.start == 1);
+ EXPECT(min.end == 9);
}
static void test_suggest_compaction_segment_nothing(void)
@@ -884,6 +839,17 @@ static void test_empty_add(void)
reftable_stack_destroy(st2);
}
+static int fastlog2(uint64_t sz)
+{
+ int l = 0;
+ if (sz == 0)
+ return 0;
+ for (; sz; sz /= 2) {
+ l++;
+ }
+ return l - 1;
+}
+
static void test_reftable_stack_auto_compaction(void)
{
struct reftable_write_options cfg = { 0 };
@@ -1072,7 +1038,6 @@ static void test_reftable_stack_compaction_concurrent_clean(void)
int stack_test_main(int argc, const char *argv[])
{
RUN_TEST(test_empty_add);
- RUN_TEST(test_log2);
RUN_TEST(test_names_equal);
RUN_TEST(test_parse_names);
RUN_TEST(test_read_file);
@@ -1092,9 +1057,6 @@ int stack_test_main(int argc, const char *argv[])
RUN_TEST(test_reftable_stack_update_index_check);
RUN_TEST(test_reftable_stack_uptodate);
RUN_TEST(test_reftable_stack_validate_refname);
- RUN_TEST(test_sizes_to_segments);
- RUN_TEST(test_sizes_to_segments_all_equal);
- RUN_TEST(test_sizes_to_segments_empty);
RUN_TEST(test_suggest_compaction_segment);
RUN_TEST(test_suggest_compaction_segment_nothing);
return 0;
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 6a131e40b81..a3b1a04123e 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -293,7 +293,7 @@ test_expect_success 'ref transaction: writes cause auto-compaction' '
test_line_count = 1 repo/.git/reftable/tables.list &&
test_commit -C repo --no-tag A &&
- test_line_count = 2 repo/.git/reftable/tables.list &&
+ test_line_count = 1 repo/.git/reftable/tables.list &&
test_commit -C repo --no-tag B &&
test_line_count = 1 repo/.git/reftable/tables.list
@@ -324,7 +324,7 @@ test_expect_success 'ref transaction: writes are synced' '
git -C repo -c core.fsync=reference \
-c core.fsyncMethod=fsync update-ref refs/heads/branch HEAD &&
check_fsync_events trace2.txt <<-EOF
- "name":"hardware-flush","count":2
+ "name":"hardware-flush","count":4
EOF
'
@@ -334,8 +334,8 @@ test_expect_success 'pack-refs: compacts tables' '
test_commit -C repo A &&
ls -1 repo/.git/reftable >table-files &&
- test_line_count = 4 table-files &&
- test_line_count = 3 repo/.git/reftable/tables.list &&
+ test_line_count = 3 table-files &&
+ test_line_count = 2 repo/.git/reftable/tables.list &&
git -C repo pack-refs &&
ls -1 repo/.git/reftable >table-files &&
@@ -367,7 +367,7 @@ do
umask $umask &&
git init --shared=true repo &&
test_commit -C repo A &&
- test_line_count = 3 repo/.git/reftable/tables.list
+ test_line_count = 2 repo/.git/reftable/tables.list
) &&
git -C repo pack-refs &&
test_expect_perms "-rw-rw-r--" repo/.git/reftable/tables.list &&
@@ -737,10 +737,10 @@ test_expect_success 'worktree: pack-refs in main repo packs main refs' '
test_commit -C repo A &&
git -C repo worktree add ../worktree &&
- test_line_count = 3 repo/.git/worktrees/worktree/reftable/tables.list &&
- test_line_count = 4 repo/.git/reftable/tables.list &&
+ test_line_count = 1 repo/.git/worktrees/worktree/reftable/tables.list &&
+ test_line_count = 1 repo/.git/reftable/tables.list &&
git -C repo pack-refs &&
- test_line_count = 3 repo/.git/worktrees/worktree/reftable/tables.list &&
+ test_line_count = 1 repo/.git/worktrees/worktree/reftable/tables.list &&
test_line_count = 1 repo/.git/reftable/tables.list
'
@@ -750,11 +750,11 @@ test_expect_success 'worktree: pack-refs in worktree packs worktree refs' '
test_commit -C repo A &&
git -C repo worktree add ../worktree &&
- test_line_count = 3 repo/.git/worktrees/worktree/reftable/tables.list &&
- test_line_count = 4 repo/.git/reftable/tables.list &&
+ test_line_count = 1 repo/.git/worktrees/worktree/reftable/tables.list &&
+ test_line_count = 1 repo/.git/reftable/tables.list &&
git -C worktree pack-refs &&
test_line_count = 1 repo/.git/worktrees/worktree/reftable/tables.list &&
- test_line_count = 4 repo/.git/reftable/tables.list
+ test_line_count = 1 repo/.git/reftable/tables.list
'
test_expect_success 'worktree: creating shared ref updates main stack' '
@@ -770,7 +770,7 @@ test_expect_success 'worktree: creating shared ref updates main stack' '
git -C worktree update-ref refs/heads/shared HEAD &&
test_line_count = 1 repo/.git/worktrees/worktree/reftable/tables.list &&
- test_line_count = 2 repo/.git/reftable/tables.list
+ test_line_count = 1 repo/.git/reftable/tables.list
'
test_expect_success 'worktree: creating per-worktree ref updates worktree stack' '
base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3] gitk: new option to hide prefetch refs
From: Tal Kelrich @ 2024-03-05 19:45 UTC (permalink / raw)
To: gitgitgadget; +Cc: git, hasturkun, paulus
In-Reply-To: <pull.1023.git.1629807526939.gitgitgadget@gmail.com>
The maintenance 'prefetch' task creates refs that mirror remote refs,
and in repositories with many branches this can clutter the commit list.
Add a new option to ignore any prefetch refs, enabled by default.
Signed-off-by: Tal Kelrich <hasturkun@gmail.com>
---
Changes since v1:
- Patch rebuilt on gitk tree.
gitk | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/gitk b/gitk
index 0ae7d68..85315df 100755
--- a/gitk
+++ b/gitk
@@ -1780,6 +1780,7 @@ proc readrefs {} {
global selecthead selectheadid
global hideremotes
global tclencoding
+ global hideprefetch
foreach v {tagids idtags headids idheads otherrefids idotherrefs} {
unset -nocomplain $v
@@ -1814,6 +1815,7 @@ proc readrefs {} {
}
set tagids($name) $id
lappend idtags($id) $name
+ } elseif {[string match "prefetch/*" $name] && $hideprefetch} {
} else {
set otherrefids($name) $id
lappend idotherrefs($id) $name
@@ -11551,7 +11553,7 @@ proc create_prefs_page {w} {
proc prefspage_general {notebook} {
global NS maxwidth maxgraphpct showneartags showlocalchanges
global tabstop limitdiffs autoselect autosellen extdifftool perfile_attrs
- global hideremotes want_ttk have_ttk maxrefs web_browser
+ global hideremotes want_ttk have_ttk maxrefs web_browser hideprefetch
set page [create_prefs_page $notebook.general]
@@ -11575,6 +11577,9 @@ proc prefspage_general {notebook} {
${NS}::checkbutton $page.hideremotes -text [mc "Hide remote refs"] \
-variable hideremotes
grid x $page.hideremotes -sticky w
+ ${NS}::checkbutton $page.hideprefetch -text [mc "Hide prefetch refs"] \
+ -variable hideprefetch
+ grid x $page.hideprefetch -sticky w
${NS}::label $page.ddisp -text [mc "Diff display options"]
grid $page.ddisp - -sticky w -pady 10
@@ -11699,7 +11704,7 @@ proc doprefs {} {
global oldprefs prefstop showneartags showlocalchanges
global uicolor bgcolor fgcolor ctext diffcolors selectbgcolor markbgcolor
global tabstop limitdiffs autoselect autosellen extdifftool perfile_attrs
- global hideremotes want_ttk have_ttk
+ global hideremotes want_ttk have_ttk hideprefetch
set top .gitkprefs
set prefstop $top
@@ -11708,7 +11713,8 @@ proc doprefs {} {
return
}
foreach v {maxwidth maxgraphpct showneartags showlocalchanges \
- limitdiffs tabstop perfile_attrs hideremotes want_ttk} {
+ limitdiffs tabstop perfile_attrs hideremotes want_ttk \
+ hideprefetch} {
set oldprefs($v) [set $v]
}
ttk_toplevel $top
@@ -11834,7 +11840,8 @@ proc prefscan {} {
global oldprefs prefstop
foreach v {maxwidth maxgraphpct showneartags showlocalchanges \
- limitdiffs tabstop perfile_attrs hideremotes want_ttk} {
+ limitdiffs tabstop perfile_attrs hideremotes want_ttk \
+ hideprefetch} {
global $v
set $v $oldprefs($v)
}
@@ -11848,7 +11855,7 @@ proc prefsok {} {
global oldprefs prefstop showneartags showlocalchanges
global fontpref mainfont textfont uifont
global limitdiffs treediffs perfile_attrs
- global hideremotes
+ global hideremotes hideprefetch
catch {destroy $prefstop}
unset prefstop
@@ -11894,7 +11901,8 @@ proc prefsok {} {
$limitdiffs != $oldprefs(limitdiffs)} {
reselectline
}
- if {$hideremotes != $oldprefs(hideremotes)} {
+ if {$hideremotes != $oldprefs(hideremotes) ||
+ $hideprefetch != $oldprefs(hideprefetch)} {
rereadrefs
}
}
@@ -12368,6 +12376,7 @@ set cmitmode "patch"
set wrapcomment "none"
set showneartags 1
set hideremotes 0
+set hideprefetch 1
set maxrefs 20
set visiblerefs {"master"}
set maxlinelen 200
@@ -12480,7 +12489,7 @@ set config_variables {
filesepbgcolor filesepfgcolor linehoverbgcolor linehoverfgcolor
linehoveroutlinecolor mainheadcirclecolor workingfilescirclecolor
indexcirclecolor circlecolors linkfgcolor circleoutlinecolor diffbgcolors
- web_browser
+ web_browser hideprefetch
}
foreach var $config_variables {
config_init_trace $var
--
2.37.1.windows.1
^ permalink raw reply related
* Tc
From: Rifani mei @ 2024-03-05 19:44 UTC (permalink / raw)
To: git
Dikirim dari iPhone saya
^ permalink raw reply
* Re: [PATCH v6 0/9] Enrich Trailer API
From: Junio C Hamano @ 2024-03-05 19:41 UTC (permalink / raw)
To: Josh Steadmon
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Randall S. Becker, Christian Couder,
Kristoffer Haugsbakk, Linus Arver
In-Reply-To: <Zedtd6esmIgayeoU@google.com>
Josh Steadmon <steadmon@google.com> writes:
> On 2024.03.05 10:03, Junio C Hamano wrote:
>>
>> It's been nearly a week since this was posted. Any more comments,
>> or is everybody happy with this iteration? Otherwise I am tempted
>> to mark the topic for 'next' soon.
>>
>> Thanks.
>
> I scanned through v6 yesterday and have nothing new to add. LGTM.
Thanks.
^ permalink raw reply
* Re: [PATCH v6 0/9] Enrich Trailer API
From: Josh Steadmon @ 2024-03-05 19:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Arver via GitGitGadget, git, Christian Couder,
Emily Shaffer, Randall S. Becker, Christian Couder,
Kristoffer Haugsbakk, Linus Arver
In-Reply-To: <xmqq5xy036a2.fsf@gitster.g>
On 2024.03.05 10:03, Junio C Hamano wrote:
>
> It's been nearly a week since this was posted. Any more comments,
> or is everybody happy with this iteration? Otherwise I am tempted
> to mark the topic for 'next' soon.
>
> Thanks.
I scanned through v6 yesterday and have nothing new to add. LGTM.
^ permalink raw reply
* Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe
From: Junio C Hamano @ 2024-03-05 18:53 UTC (permalink / raw)
To: Atneya Nair; +Cc: git, jeffhost, me, nasamuffin, Tanay Abhra, Glen Choo
In-Reply-To: <xmqqwmqg38u2.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> The use of strintern() comes originally from 3df8fd62 ...
> ..., so they may
> know how safe the change on the config side would be (I still do
> not understand why you'd want to do this in the first place, though,
> especially if you are protecting the callsites with mutex).
The risks of turning code that uses strintern() to use strdup() are
* you will leak the allocated string unless you explicitly free the
string you now own.
* you may consume too much memory if you are creating too many
copies of the same string (e.g. if you need filename for each
line in a file in an application, the memory consumption can
become 1000-fold).
* the code may be taking advantage of the fact that two such
strings can be compared for (in)equality simply by comparing
their addresses, which you would need to adjust to use !strcmp()
and the like.
I just checked to make sure that the last one is not the case for
our codebase, and you said you didn't see the second one is the case,
so the change may be a safe one to make.
One more thing. Do not use strdup() without checking its return
value for failure. It would be an easy fix to use xstrdup() instead.
Thanks.
>> diff --git a/config.c b/config.c
>> index 3cfeb3d8bd..d7f73d8745 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
>> enum config_scope scope,
>> struct key_value_info *out)
>> {
>> - out->filename = strintern(cs->name);
>> + out->filename = strdup(cs->name);
>> out->origin_type = cs->origin_type;
>> out->linenr = cs->linenr;
>> out->scope = scope;
>> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
>>
>> strbuf_release(&top->value);
>> strbuf_release(&top->var);
>> + free(kvi.filename);
>>
>> return ret;
>> }
^ permalink raw reply
* Re: [PATCH v6 0/9] Enrich Trailer API
From: Junio C Hamano @ 2024-03-05 18:03 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Christian Couder, Kristoffer Haugsbakk,
Linus Arver
In-Reply-To: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This patch series is the first 9 patches of a larger cleanup/bugfix series
> (henceforth "larger series") I've been working on. The main goal of this
> series is to begin the process of "libifying" the trailer API. By "API" I
> mean the interface exposed in trailer.h. The larger series brings a number
> of additional cleanups (exposing and fixing some bugs along the way), and
> builds on top of this series.
>
> When the larger series is merged, we will be in a good state to additionally
> pursue the following goals:
>
> 1. "API reuse inside Git": make the API expressive enough to eliminate any
> need by other parts of Git to use the interpret-trailers builtin as a
> subprocess (instead they could just use the API directly);
> 2. "API stability": add unit tests to codify the expected behavior of API
> functions; and
> 3. "API documentation": create developer-focused documentation to explain
> how to use the API effectively, noting any API limitations or
> anti-patterns.
>
> In the future after libification is "complete", users external to Git will
> be able to use the same trailer processing API used by the
> interpret-trailers builtin. For example, a web server may want to parse
> trailers the same way that Git would parse them, without having to call
> interpret-trailers as a subprocess. This use case was the original
> motivation behind my work in this area.
>
> With the libification-focused goals out of the way, let's turn to this patch
> series in more detail.
>
> In summary this series breaks up "process_trailers()" into smaller pieces,
> exposing many of the parts relevant to trailer-related processing in
> trailer.h. This will force us to eventually introduce unit tests for these
> API functions, but that is a good thing for API stability. We also perform
> some preparatory refactors in order to help us unify the trailer formatting
> machinery toward the end of this series.
>
>
> Notable changes in v6
> =====================
>
> * Mainly wording changes to commit messages. Thanks to Christian for the
> suggestions.
It's been nearly a week since this was posted. Any more comments,
or is everybody happy with this iteration? Otherwise I am tempted
to mark the topic for 'next' soon.
Thanks.
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Kristoffer Haugsbakk @ 2024-03-05 17:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Manlio Perillo, git, Dragan Simic
In-Reply-To: <xmqqedco37hh.fsf@gitster.g>
On Tue, Mar 5, 2024, at 18:37, Junio C Hamano wrote:
>> Maybe introduce `core.commentString` and make it a synonym for
>> `core.commentChar`?
>
> Yes, if we were to do so. As I already said, this is not my itch,
> but such a synonym would be part of the migration plan if somebody
> seriously designs this as a new feature.
Maybe someone will discover an itch:
https://github.com/gitgitgadget/git/issues/1685
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Junio C Hamano @ 2024-03-05 17:37 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Manlio Perillo, git
In-Reply-To: <9633f9be5ddd9ab3df4b79ee934e1ed47e90bd1d.1709656683.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
>> I personally do not see a reason, however, why we need to be limited
>> to a single byte, though. If a patch cleanly implements to allow us
>> to use any one-or-more-byte sequence as core.commentChar, I do not
>> offhand see a good reason to reject it---it would be fully backward
>> compatible and allows you to use a UTF-8 charcter outside ASCII, as
>> well as "//" and the like.
>
> Allow one codepoint or a string?
I said "any one-or-more-byte sequence" and I meant it. It does not
even have to be a full and complete UTF-8 character. As long as we
correctly prefix the sequence and strip it from the front, I do not
care if the user chooses to use a broken half-character ;-).
> Maybe introduce `core.commentString` and make it a synonym for
> `core.commentChar`?
Yes, if we were to do so. As I already said, this is not my itch,
but such a synonym would be part of the migration plan if somebody
seriously designs this as a new feature.
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index 0e8c2832bf9..2d4bbdb25fa 100644
> --- a/Documentation/config/core.txt
> +++ b/Documentation/config/core.txt
> @@ -521,7 +521,7 @@ core.editor::
>
> core.commentChar::
> Commands such as `commit` and `tag` that let you edit
> - messages consider a line that begins with this character
> + messages consider a line that begins with this ASCII character
> commented, and removes them after the editor returns
> (default '#').
> +
Looks sensible. Thanks. Will queue.
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Kristoffer Haugsbakk @ 2024-03-05 17:37 UTC (permalink / raw)
To: Dragan Simic; +Cc: Manlio Perillo, git, Junio C Hamano
In-Reply-To: <1e71ce757c3d773fd7354cd12473b851@manjaro.org>
On Tue, Mar 5, 2024, at 18:20, Dragan Simic wrote:
> On 2024-03-05 17:58, Kristoffer Haugsbakk wrote:
>> Personally I think it’s okay. `%` for example is a good candidate since
>> you seldom use that as a leading character in prose (after a
>> whitespace), and it seems that `%` is often recommended as an
>> alternative.
>
> Isn't '%' actually an ASCII character?
I wasn’t clear: personally I think the status quo of only allowing ASCII
characters seems fine given that you can use something like `%` as an
alternative comment char.
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Dragan Simic @ 2024-03-05 17:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, Manlio Perillo, git
In-Reply-To: <xmqqmsrc4osm.fsf@gitster.g>
On 2024-03-05 17:38, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>> On 2024-03-05 16:32, Junio C Hamano wrote:
>>> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>>>> I think this is more about `git config --add` not doing any
>>>> validation. It just sets things. You can do `git config --add
>>>> core.commentChar 'ffd'` and get the same effect.
>>> As you said, we should document core.commentChar as limited to an
>>> ASCII character, at least as a short term solution.
>>> I personally do not see a reason, however, why we need to be limited
>>> to a single byte, though. If a patch cleanly implements to allow us
>>> to use any one-or-more-byte sequence as core.commentChar, I do not
>>> offhand see a good reason to reject it---it would be fully backward
>>> compatible and allows you to use a UTF-8 charcter outside ASCII, as
>>> well as "//" and the like.
>>
>> May I ask why would we want the comment character to possibly be
>> a multibyte character? I mean, I support localization, to make it all
>> easier for the users who opt not to use English, but wouldn't allowing
>> multibyte characters for the comment character simply be a bit
>> unneeded?
>>
>> Maybe I'm missing something?
>
> That's not a question for me ;-).
>
> It is not my personal itch, so I haven't done anything to make the
> commentChar take more than one byte. But if it is somebody else's
> itch, I do not see a reason why we should forbid them from
> scratching. If the setting seeps through across repository
> boundaries, that may create a compatibility issue and that by itself
> might be such a reason. If it greatly makes the code more complex,
> that may be another reason you can use to argue against adding such
> a "feature". If it makes the semantics of what "a comment string"
> is and how they are added and stripped at various stages of
> processing commit log messages fuzzy and harder to document and
> understand, that might be another reason. I however do not think
> any of these to be true. Maybe I am overly optimistic. I haven't
> looked deeply into the code around commentChar for quite some time.
Yes, there are quite a few possible obstacles. As I replied to
Kristoffer
a bit earlier, I see this more as a programming exercise. Of course,
unless someone really needs it as a new feature, in which case they will
probably need to overcome all those obstacles. :)
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Dragan Simic @ 2024-03-05 17:20 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Manlio Perillo, git, Junio C Hamano
In-Reply-To: <3bef4fef-0a00-4ed8-878e-372c4d8f552d@app.fastmail.com>
On 2024-03-05 17:58, Kristoffer Haugsbakk wrote:
> On Tue, Mar 5, 2024, at 16:42, Dragan Simic wrote:
>> May I ask why would we want the comment character to possibly be
>> a multibyte character? I mean, I support localization, to make it all
>> easier for the users who opt not to use English, but wouldn't allowing
>> multibyte characters for the comment character simply be a bit
>> unneeded?
>>
>> Maybe I'm missing something?
>
> Personally I think it’s okay. `%` for example is a good candidate since
> you seldom use that as a leading character in prose (after a
> whitespace), and it seems that `%` is often recommended as an
> alternative.
Isn't '%' actually an ASCII character?
> But if it doesn’t make the code more complex: why not? (I just
> personally don’t have a use-case.)
Frankly, allowing multibyte characters as comment characters looks to me
more like a programming exercise than a really needed feature.
^ permalink raw reply
* Re: Clarify the meaning of "character" in the documentation
From: Junio C Hamano @ 2024-03-05 16:38 UTC (permalink / raw)
To: Dragan Simic; +Cc: Kristoffer Haugsbakk, Manlio Perillo, git
In-Reply-To: <52d6850914982ffaf15dda937d611ffb@manjaro.org>
Dragan Simic <dsimic@manjaro.org> writes:
> On 2024-03-05 16:32, Junio C Hamano wrote:
>> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>>> I think this is more about `git config --add` not doing any
>>> validation. It just sets things. You can do `git config --add
>>> core.commentChar 'ffd'` and get the same effect.
>> As you said, we should document core.commentChar as limited to an
>> ASCII character, at least as a short term solution.
>> I personally do not see a reason, however, why we need to be limited
>> to a single byte, though. If a patch cleanly implements to allow us
>> to use any one-or-more-byte sequence as core.commentChar, I do not
>> offhand see a good reason to reject it---it would be fully backward
>> compatible and allows you to use a UTF-8 charcter outside ASCII, as
>> well as "//" and the like.
>
> May I ask why would we want the comment character to possibly be
> a multibyte character? I mean, I support localization, to make it all
> easier for the users who opt not to use English, but wouldn't allowing
> multibyte characters for the comment character simply be a bit unneeded?
>
> Maybe I'm missing something?
That's not a question for me ;-).
It is not my personal itch, so I haven't done anything to make the
commentChar take more than one byte. But if it is somebody else's
itch, I do not see a reason why we should forbid them from
scratching. If the setting seeps through across repository
boundaries, that may create a compatibility issue and that by itself
might be such a reason. If it greatly makes the code more complex,
that may be another reason you can use to argue against adding such
a "feature". If it makes the semantics of what "a comment string"
is and how they are added and stripped at various stages of
processing commit log messages fuzzy and harder to document and
understand, that might be another reason. I however do not think
any of these to be true. Maybe I am overly optimistic. I haven't
looked deeply into the code around commentChar for quite some time.
^ permalink raw reply
* Re: [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe
From: Junio C Hamano @ 2024-03-05 17:08 UTC (permalink / raw)
To: Atneya Nair; +Cc: git, jeffhost, me, nasamuffin, Tanay Abhra, Glen Choo
In-Reply-To: <20240305012112.1598053-4-atneya@google.com>
Atneya Nair <atneya@google.com> writes:
> To enable parallel update of the read cache for submodules,
> ce_compare_gitlink must be thread safe (for different objects).
>
> Remove string interning in do_config_from (called from
> repo_submodule_init) and add locking around accessing the ref_store_map.
This step does two independent things, even though they may have
dependencies, i.e., for one to be a solution for the problem it is
tackling, the other may have to be there already. E.g., even after
calls to ce_compare_gitlink() get serialized via a mutex, it may for
some reason not work without giving each kvi.filename its own copy
[*], and if that is the case, you may need to have the "stop
interning" step in a single patch with its own justification, and
then "have mutex around ref_store calls" patch has to come after it.
Side note: I do not know if that is the case myself. I didn't
write this commit, you did. The above is just a sample to
illustrate the expected level of depth to explain your thinking
in the log message.
Or if these two things must happen at the same time, please explain
in the proposed log message why they have to happen in the same
commit. The two paragraphs you wrote there don't explain that, so I
am assuming that it is not the case.
The use of strintern() comes originally from 3df8fd62 (add line
number and file name info to `config_set`, 2014-08-07) by Tanay
Abhra <tanayabh@gmail.com>, and survived a handful of changes
809d8680 (config: pass ctx with config files, 2023-06-28)
a798a56c (config.c: plumb the_reader through callbacks, 2023-03-28)
c97f3ed2 (config.c: plumb config_source through static fns, 2023-03-28)
all of which were done by Glen Choo <glencbz@gmail.com>, so they may
know how safe the change on the config side would be (I still do
not understand why you'd want to do this in the first place, though,
especially if you are protecting the callsites with mutex).
I also think Emily's (who you already have on the "CC:" line) group
wants to libify the config machinery and suspect they may still be
making changes to the code, so you may want to coordinate with them
to avoid duplicated work and overlapping changes.
> Signed-off-by: Atneya Nair <atneya@google.com>
> ---
>
> Notes:
> Chasing down thread unsafe code was done using tsan.
Very nice to know.
> config.c | 3 ++-
> config.h | 2 +-
> refs.c | 9 +++++++++
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 3cfeb3d8bd..d7f73d8745 100644
> --- a/config.c
> +++ b/config.c
> @@ -1017,7 +1017,7 @@ static void kvi_from_source(struct config_source *cs,
> enum config_scope scope,
> struct key_value_info *out)
> {
> - out->filename = strintern(cs->name);
> + out->filename = strdup(cs->name);
> out->origin_type = cs->origin_type;
> out->linenr = cs->linenr;
> out->scope = scope;
> @@ -1857,6 +1857,7 @@ static int do_config_from(struct config_source *top, config_fn_t fn,
>
> strbuf_release(&top->value);
> strbuf_release(&top->var);
> + free(kvi.filename);
>
> return ret;
> }
> diff --git a/config.h b/config.h
> index 5dba984f77..b78f1b6667 100644
> --- a/config.h
> +++ b/config.h
> @@ -118,7 +118,7 @@ struct config_options {
>
> /* Config source metadata for a given config key-value pair */
> struct key_value_info {
> - const char *filename;
> + char *filename;
> int linenr;
> enum config_origin_type origin_type;
> enum config_scope scope;
> diff --git a/refs.c b/refs.c
> index c633abf284..cce8a31b22 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2126,6 +2126,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> size_t len;
> struct repository *subrepo;
>
> + // TODO is this locking tolerable, and/or can we get any finer
> + static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> +
> if (!submodule)
> return NULL;
>
> @@ -2139,7 +2142,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> /* We need to strip off one or more trailing slashes */
> submodule = to_free = xmemdupz(submodule, len);
>
> + pthread_mutex_lock(&lock);
> refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
> + pthread_mutex_unlock(&lock);
> if (refs)
> goto done;
>
> @@ -2162,10 +2167,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> free(subrepo);
> goto done;
> }
> +
> + pthread_mutex_lock(&lock);
> + // TODO maybe lock this separately
> refs = ref_store_init(subrepo, submodule_sb.buf,
> REF_STORE_READ | REF_STORE_ODB);
> register_ref_store_map(&submodule_ref_stores, "submodule",
> refs, submodule);
> + pthread_mutex_unlock(&lock);
>
> done:
> strbuf_release(&submodule_sb);
^ permalink raw reply
* Re: [PATCH] show-ref: add --unresolved option
From: Kristoffer Haugsbakk @ 2024-03-05 17:01 UTC (permalink / raw)
To: Phillip Wood; +Cc: John Cai, Josh Soref, git
In-Reply-To: <a3de2b7b-4603-4604-a4d2-938a598e312e@gmail.com>
On Tue, Mar 5, 2024, at 16:30, Phillip Wood wrote:
> Hi John
>
> On 04/03/2024 22:51, John Cai via GitGitGadget wrote:
>> From: John Cai <johncai86@gmail.com>
>>
>> For reftable development, it would be handy to have a tool to provide
>> the direct value of any ref whether it be a symbolic ref or not.
>> Currently there is git-symbolic-ref, which only works for symbolic refs,
>> and git-rev-parse, which will resolve the ref. Let's add a --unresolved
>> option that will only take one ref and return whatever it points to
>> without dereferencing it.
>
> "--unresolved" makes me think of merge conflicts. I wonder if
> "--no-dereference" would be clearer.
Yeah, a `--no`-style option looks more consistent.
^ 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