* Re: [PATCH v2] setup: remove unnecessary variable
From: Junio C Hamano @ 2024-03-04 21:53 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: christian.couder, git, johannes.schindelin, newren
In-Reply-To: <CZLA8TF4XG5S.KU06P62V03TV@gmail.com>
"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
> Yes, in the previous testcase we determined that template cannot set
> core.bare. Therefore, this testcase would be like testing
> --bare/--no-bare option, which is already done in 0001-init.sh and
> t5601-clone.sh. However, I don't have strong opinion on this. I can add
> it back if you think it is worth it.
I was merely trying to make sure that I understood your motivation
behind the change, which was described at the end of the commit log
message, i.e. "... and remove one redundant testcase.".
Thanks.
^ permalink raw reply
* [PATCH v3 0/5] advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-04 22:07 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Elijah Newren, Jean-Noël Avila
In-Reply-To: <4ad5d4190649dcb5f26c73a6f15ab731891b9dfd.1709491818.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 previous cover letter)
§ 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 v3
• New preliminary patches 1–4
• Fix test style
• Improvements to `advice.txt` (style consistency and other)
• Patch 5/5:
• Tweak advice doc for the new entry
• Better test style
Kristoffer Haugsbakk (5):
t3200: improve test style
advice: make all entries stylistically consistent
advice: use backticks for code
advice: use double quotes for regular quoting
branch: advise about ref syntax rules
Documentation/config/advice.txt | 91 ++++++++++++-----------
advice.c | 1 +
advice.h | 1 +
branch.c | 8 +-
builtin/branch.c | 8 +-
t/t3200-branch.sh | 125 +++++++++++++++++---------------
6 files changed, 127 insertions(+), 107 deletions(-)
Range-diff against v2:
-: ----------- > 1: e6a2628ce57 t3200: improve test style
-: ----------- > 2: d48b4719c27 advice: make all entries stylistically consistent
-: ----------- > 3: 30d662a04c7 advice: use backticks for code
-: ----------- > 4: 3028713357f advice: use double quotes for regular quoting
1: 4ad5d419064 ! 5: 402b7937951 branch: advise about ref syntax rules
@@ Commit message
## Notes (series) ##
+ 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)`
@@ Notes (series)
## Documentation/config/advice.txt ##
@@ Documentation/config/advice.txt: advice.*::
- 'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
- 'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
+ `pushNonFFCurrent`, `pushNonFFMatching`, `pushAlreadyExists`,
+ `pushFetchFirst`, `pushNeedsForce`, and `pushRefNeedsUpdate`
simultaneously.
+ refSyntax::
-+ Point the user towards the ref syntax documentation if
-+ they give an invalid ref name.
++ Shown when the user provides an illegal ref name: point
++ towards the ref syntax documentation.
resetNoRefresh::
- Advice to consider using the `--no-refresh` option to
- linkgit:git-reset[1] when the command takes more than 2 seconds
+ Shown when linkgit:git-reset[1] takes more than 2
+ seconds to refresh the index after reset: tell the user
## advice.c ##
@@ advice.c: static struct {
@@ t/t3200-branch.sh: test_expect_success '--track overrides branch.autoSetupMerge'
test_cmp_config "" --default "" branch.foo5.merge
'
-+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_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
+'
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply
* [PATCH v3 1/5] t3200: improve test style
From: Kristoffer Haugsbakk @ 2024-03-04 22:07 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Junio C Hamano
In-Reply-To: <cover.1709590037.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
• 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>
---
t/t3200-branch.sh | 115 ++++++++++++++++++++++------------------------
1 file changed, 56 insertions(+), 59 deletions(-)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index de7d3014e4f..273a57a72d8 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' '
+test_expect_success '(setup) 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 v3 2/5] advice: make all entries stylistically consistent
From: Kristoffer Haugsbakk @ 2024-03-04 22:07 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Junio C Hamano
In-Reply-To: <cover.1709590037.git.code@khaugsbakk.name>
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*
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
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?
Documentation/config/advice.txt | 80 ++++++++++++++++-----------------
1 file changed, 40 insertions(+), 40 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index c7ea70f2e2e..cfca87a6aa2 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: tell 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
+ to move to the detached HEAD state; instruct 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: 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: 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]:
+ 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: consider using 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: instruct how to create a new unborn
branch instead.
--
--
2.44.0.64.g52b67adbeb2
^ permalink raw reply related
* [PATCH v3 3/5] advice: use backticks for code
From: Kristoffer Haugsbakk @ 2024-03-04 22:07 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709590037.git.code@khaugsbakk.name>
Use backticks for quoting code rather than single quotes.
Also replace “the add command” with “`git add`”.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Documentation/config/advice.txt | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index cfca87a6aa2..df447dd5d14 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::
@@ -90,9 +90,9 @@ advice.*::
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 v3 4/5] advice: use double quotes for regular quoting
From: Kristoffer Haugsbakk @ 2024-03-04 22:07 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709590037.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 df447dd5d14..c5d3d6790a5 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 v3 5/5] branch: advise about ref syntax rules
From: Kristoffer Haugsbakk @ 2024-03-04 22:07 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk
In-Reply-To: <cover.1709590037.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):
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 c5d3d6790a5..06a3a3cc9b5 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: point
+ towards 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
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 273a57a72d8..30a97e3776e 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
* What's cooking in git.git (Mar 2024, #01; Mon, 4)
From: Junio C Hamano @ 2024-03-04 22:36 UTC (permalink / raw)
To: git
Here are the topics that have been cooking in my tree. Commits
prefixed with '+' are in 'next' (being in 'next' is a sign that a
topic is stable enough to be used and are candidate to be in a
future release). Commits prefixed with '-' are only in 'seen', and
aren't considered "accepted" at all and may be annotated with an URL
to a message that raises issues but they are no means exhaustive. A
topic without enough support may be discarded after a long period of
no activity (of course they can be resubmit when new interests
arise).
Copies of the source code to Git live in many repositories, and the
following is a list of the ones I push into or their mirrors. Some
repositories have only a subset of branches.
With maint, master, next, seen, todo:
git://git.kernel.org/pub/scm/git/git.git/
git://repo.or.cz/alt-git.git/
https://kernel.googlesource.com/pub/scm/git/git/
https://github.com/git/git/
https://gitlab.com/git-vcs/git/
With all the integration branches and topics broken out:
https://github.com/gitster/git/
Even though the preformatted documentation in HTML and man format
are not sources, they are published in these repositories for
convenience (replace "htmldocs" with "manpages" for the manual
pages):
git://git.kernel.org/pub/scm/git/git-htmldocs.git/
https://github.com/gitster/git-htmldocs.git/
Release tarballs are available at:
https://www.kernel.org/pub/software/scm/git/
--------------------------------------------------
[Graduated to 'master']
* cp/t9146-use-test-path-helpers (2024-02-14) 1 commit
(merged to 'next' on 2024-02-21 at 0b8356ef33)
+ t9146: replace test -d/-e/-f with appropriate test_path_is_* function
Test script clean-up.
source: <pull.1661.v3.git.1707933048210.gitgitgadget@gmail.com>
* ds/doc-send-email-capitalization (2024-02-20) 1 commit
(merged to 'next' on 2024-02-21 at c4aac4b993)
+ documentation: send-email: use camel case consistently
Doc update.
source: <88f1fe08c3047e14090957093ee8d98b0f60cb6c.1708467601.git.dsimic@manjaro.org>
* hs/rebase-not-in-progress (2024-02-21) 1 commit
(merged to 'next' on 2024-02-23 at ce53f5243f)
+ rebase: make warning less passive aggressive
Error message update.
source: <pull.1669.v2.git.1708537097448.gitgitgadget@gmail.com>
* ja/doc-placeholders-markup-rules (2024-02-21) 1 commit
(merged to 'next' on 2024-02-23 at b6761ceeac)
+ doc: clarify the format of placeholders
The way placeholders are to be marked-up in documentation have been
specified; use "_<placeholder>_" to typeset the word inside a pair
of <angle-brakets> emphasized.
source: <pull.1671.git.1708550340094.gitgitgadget@gmail.com>
* ja/docfixes (2024-02-20) 3 commits
(merged to 'next' on 2024-02-21 at 6d778ca672)
+ doc: end sentences with full-stop
+ doc: close unclosed angle-bracket of a placeholder in git-clone doc
+ doc: git-rev-parse: enforce command-line description syntax
Doc update.
source: <pull.1670.git.1708468374.gitgitgadget@gmail.com>
* jc/doc-add-placeholder-fix (2024-02-21) 1 commit
(merged to 'next' on 2024-02-23 at 6950ff216b)
+ doc: apply the new placeholder rules to git-add documentation
Practice the new mark-up rule for <placeholders> with "git add"
documentation page.
source: <xmqqbk89molz.fsf@gitster.g>
* jw/remote-doc-typofix (2024-02-21) 1 commit
(merged to 'next' on 2024-02-23 at fe95873cfa)
+ git-remote.txt: fix typo
Docfix.
source: <20240221083554.5255-1-jwilk@jwilk.net>
* ps/difftool-dir-diff-exit-code (2024-02-20) 1 commit
(merged to 'next' on 2024-02-21 at a7bbef5a5f)
+ git-difftool--helper: honor `--trust-exit-code` with `--dir-diff`
"git difftool --dir-diff" learned to honor the "--trust-exit-code"
option; it used to always exit with 0 and signalled success.
Acked-by: David Aguilar <davvid@gmail.com>
cf. <CAJDDKr5+3jszG=psh=kUGDjNCeTDGPSS-qDuN=JAq-3ua=bNDg@mail.gmail.com>
source: <0fac668f8fc021af9f9c4df5134da59816307ccc.1708423309.git.ps@pks.im>
* ps/reflog-list (2024-02-21) 9 commits
(merged to 'next' on 2024-02-23 at 7413632239)
+ builtin/reflog: introduce subcommand to list reflogs
+ refs: stop resolving ref corresponding to reflogs
+ refs: drop unused params from the reflog iterator callback
+ refs: always treat iterators as ordered
+ refs/files: sort merged worktree and common reflogs
+ refs/files: sort reflogs returned by the reflog iterator
+ dir-iterator: support iteration in sorted order
+ dir-iterator: pass name to `prepare_next_entry_data()` directly
+ Merge branch 'ps/reftable-backend' into ps/reflog-list
"git reflog" learned a "list" subcommand that enumerates known reflogs.
source: <cover.1708518982.git.ps@pks.im>
* tb/multi-pack-verbatim-reuse (2024-02-23) 1 commit
(merged to 'next' on 2024-02-23 at 8ee07f1da4)
+ Documentation/config/pack.txt: fix broken AsciiDoc mark-up
Docfix.
source: <72bb58e5f3b8a5a622394c5ff40426156e122580.1708720255.git.me@ttaylorr.com>
--------------------------------------------------
[New Topics]
* jk/upload-pack-bounded-resources (2024-02-28) 9 commits
(merged to 'next' on 2024-03-01 at b70b6f0913)
+ upload-pack: free tree buffers after parsing
+ upload-pack: use PARSE_OBJECT_SKIP_HASH_CHECK in more places
+ upload-pack: always turn off save_commit_buffer
+ upload-pack: disallow object-info capability by default
+ upload-pack: accept only a single packfile-uri line
+ upload-pack: use a strmap for want-ref lines
+ upload-pack: use oidset for deepen_not list
+ upload-pack: switch deepen-not list to an oid_array
+ upload-pack: drop separate v2 "haves" array
Originally merged to 'next' on 2024-02-29
Various parts of upload-pack has been updated to bound the resource
consumption relative to the size of the repository to protect from
abusive clients.
Will merge to 'master'.
source: <20240228223700.GA1157826@coredump.intra.peff.net>
* jk/upload-pack-v2-capability-cleanup (2024-02-29) 4 commits
(merged to 'next' on 2024-03-01 at 2750893db7)
+ upload-pack: only accept packfile-uris if we advertised it
+ upload-pack: use existing config mechanism for advertisement
+ upload-pack: centralize setup of sideband-all config
+ upload-pack: use repository struct to get config
Originally merged to 'next' on 2024-02-29
The upload-pack program, when talking over v2, accepted the
packfile-uris protocol extension from the client, even if it did
not advertise the capability, which has been corrected.
Will merge to 'master'.
source: <20240228224625.GA1158651@coredump.intra.peff.net>
* es/config-doc-sort-sections (2024-02-29) 1 commit
(merged to 'next' on 2024-03-04 at 0752144ed7)
+ docs: sort configuration variable groupings alphabetically
Doc updates.
Will merge to 'master'.
source: <20240229190229.20222-1-ericsunshine@charter.net>
* kh/doc-dashed-commands-have-not-worked-for-a-long-time (2024-03-01) 1 commit
(merged to 'next' on 2024-03-04 at 7e070c67f9)
+ gitcli: drop mention of “non-dashed form”
Doc update.
Will merge to 'master'.
source: <5b34bc4e22816f7f19bd26c15a08fe4c749b72f8.1709316230.git.code@khaugsbakk.name>
* jc/xwrite-cleanup (2024-03-02) 3 commits
- repack: check error writing to pack-objects subprocess
- sideband: avoid short write(2)
- unpack: replace xwrite() loop with write_in_full()
Uses of xwrite() helper have been audited and updated for better
error checking and simpler code.
source: <20240302190348.3946569-1-gitster@pobox.com>
* jc/test-i18ngrep (2024-03-02) 1 commit
- test_i18ngrep: hard deprecate and forbid its use
With release 2.44 we got rid of all uses of test_i18ngrep and there
is no in-flight topic that adds a new use of it. Make a call to
test_i18ngrep a hard failure, so that we can remove it at the end
of this release cycle.
Comments?
source: <xmqq5xy4zhdc.fsf@gitster.g>
* gt/core-bare-in-templates (2024-03-04) 1 commit
- setup: remove unnecessary variable
Code simplification.
Will merge to 'next'?
source: <20240304151811.511780-1-shyamthakkar001@gmail.com>
* so/clean-dry-run-without-force (2024-03-04) 2 commits
- clean: further clean-up of implementation around "--force"
- clean: improve -n and -f implementation and documentation
The implementation in "git clean" that makes "-n" and "-i" ignore
clean.requireForce has been simplified, together with the
documentation.
Will merge to 'next'.
source: <87le6ziqzb.fsf_-_@osv.gnss.ru>
source: <20240303220600.2491792-1-gitster@pobox.com>
* jh/trace2-missing-def-param-fix (2024-03-04) 4 commits
- trace2: remove unneeded calls to generate 'def_param' set
- trace2: emit 'def_param' set with 'cmd_name' event
- trace2: avoid emitting 'def_param' set more than once
- t0211: demonstrate missing 'def_param' events for certain commands
Some trace2 events that lacked def_param have learned to show it,
enriching the output.
Needs review.
source: <pull.1679.git.1709566808.gitgitgadget@gmail.com>
* ps/reftable-stack-tempfile (2024-03-04) 4 commits
- reftable/stack: register compacted tables as tempfiles
- reftable/stack: register lockfiles during compaction
- reftable/stack: register new tables as tempfiles
- lockfile: report when rollback fails
The code in reftable backend that creates new table files works
better with the tempfile framework to avoid leaving cruft after a
failure.
Needs review.
source: <cover.1709549619.git.ps@pks.im>
* rs/opt-parse-long-fixups (2024-03-03) 6 commits
- parse-options: rearrange long_name matching code
- parse-options: normalize arg and long_name before comparison
- parse-options: detect ambiguous self-negation
- parse-options: factor out register_abbrev() and struct parsed_option
- parse-options: set arg of abbreviated option lazily
- parse-options: recognize abbreviated negated option with arg
The parse-options code that deals with abbreviated long option
names have been cleaned up.
Needs review.
source: <20240303121944.20627-1-l.s.r@web.de>
* sj/t9117-path-is-file (2024-03-04) 1 commit
(merged to 'next' on 2024-03-04 at de5f6a74cb)
+ t9117: prefer test_path_* helper functions
GSoC practice to replace "test -f" with "test_path_is_file".
Will merge to 'master'.
source: <20240304095436.56399-2-shejialuo@gmail.com>
* vm/t7301-use-test-path-helpers (2024-03-04) 1 commit
- t7301: use test_path_is_(missing|file)
GSoC practice to replace "test -f" with "test_path_is_file".
Will merge to 'next'?
source: <20240304171732.64457-2-vincenzo.mezzela@gmail.com>
--------------------------------------------------
[Cooking]
* cw/git-std-lib (2024-02-28) 4 commits
- SQUASH??? get rid of apparent debugging crufts
- test-stdlib: show that git-std-lib is independent
- git-std-lib: introduce Git Standard Library
- pager: include stdint.h because uintmax_t is used
Split libgit.a out to a separate git-std-lib tor easier reuse.
Needs review.
source: <cover.1696021277.git.jonathantanmy@google.com>
* js/merge-base-with-missing-commit (2024-02-29) 11 commits
(merged to 'next' on 2024-03-01 at 3e3eabaee9)
+ commit-reach(repo_get_merge_bases_many_dirty): pass on errors
+ commit-reach(repo_get_merge_bases_many): pass on "missing commits" errors
+ commit-reach(get_octopus_merge_bases): pass on "missing commits" errors
+ commit-reach(repo_get_merge_bases): pass on "missing commits" errors
+ commit-reach(get_merge_bases_many_0): pass on "missing commits" errors
+ commit-reach(merge_bases_many): pass on "missing commits" errors
+ commit-reach(paint_down_to_common): start reporting errors
+ commit-reach(paint_down_to_common): prepare for handling shallow commits
+ commit-reach(repo_in_merge_bases_many): report missing commits
+ commit-reach(repo_in_merge_bases_many): optionally expect missing commits
+ commit-reach(paint_down_to_common): plug two memory leaks
Originally merged to 'next' on 2024-02-29
Make sure failure return from merge_bases_many() is properly caught.
Needs an incremental fix-up.
cf.<20240301065805.GB2680308@coredump.intra.peff.net>
source: <pull.1657.v4.git.1709113457.gitgitgadget@gmail.com>
* eg/add-uflags (2024-02-29) 1 commit
(merged to 'next' on 2024-03-01 at 5dbc997561)
+ add: use unsigned type for collection of bits
Originally merged to 'next' on 2024-02-29
Code clean-up practice.
Will merge to 'master'.
source: <20240229194444.8499-2-giganteeugenio2@gmail.com>
* jc/doc-compat-util (2024-02-27) 1 commit
(merged to 'next' on 2024-03-01 at 89b76c65d7)
+ doc: clarify the wording on <git-compat-util.h> requirement
Originally merged to 'next' on 2024-02-27
Clarify wording in the CodingGuidelines that requires <git-compat-util.h>
to be the first header file.
Will merge to 'master'.
source: <xmqqle76kdpr.fsf_-_@gitster.g>
* jc/no-include-of-compat-util-from-headers (2024-02-24) 1 commit
(merged to 'next' on 2024-03-01 at ebb921087e)
+ compat: drop inclusion of <git-compat-util.h>
Originally merged to 'next' on 2024-02-26
Header file clean-up.
Will merge to 'master'.
source: <xmqqwmqtli18.fsf@gitster.g>
* jk/reflog-special-cases-fix (2024-02-26) 3 commits
(merged to 'next' on 2024-03-01 at 2b67f6e668)
+ read_ref_at(): special-case ref@{0} for an empty reflog
+ get_oid_basic(): special-case ref@{n} for oldest reflog entry
+ Revert "refs: allow @{n} to work with n-sized reflog"
Originally merged to 'next' on 2024-02-27
The logic to access reflog entries by date and number had ugly
corner cases at the boundaries, which have been cleaned up.
Will merge to 'master'.
source: <20240226100010.GA1214708@coredump.intra.peff.net>
* jk/textconv-cache-outside-repo-fix (2024-02-26) 1 commit
(merged to 'next' on 2024-03-01 at 8508b83758)
+ userdiff: skip textconv caching when not in a repository
Originally merged to 'next' on 2024-02-26
The code incorrectly attempted to use textconv cache when asked,
even when we are not running in a repository, which has been
corrected.
Will merge to 'master'.
source: <20240226102729.GB2685773@coredump.intra.peff.net>
* js/remove-cruft-files (2024-02-26) 1 commit
(merged to 'next' on 2024-03-01 at 63100a274b)
+ neue: remove a bogus empty file
Originally merged to 'next' on 2024-02-26
Remove an empty file that shouldn't have been added in the first
place.
Will merge to 'master'.
source: <pull.1674.git.1708958183225.gitgitgadget@gmail.com>
* jt/commit-redundant-scissors-fix (2024-02-27) 2 commits
(merged to 'next' on 2024-03-01 at e5983498f1)
+ commit: unify logic to avoid multiple scissors lines when merging
+ commit: avoid redundant scissor line with --cleanup=scissors -v
Originally merged to 'next' on 2024-02-29
"git commit -v --cleanup=scissors" used to add the scissors line
twice in the log message buffer, which has been corrected.
Will merge to 'master'.
source: <Zd2eLxPelxvP8FDk@localhost>
* pb/ort-make-submodule-conflict-message-an-advice (2024-02-26) 1 commit
(merged to 'next' on 2024-03-01 at df880cde2e)
+ merge-ort: turn submodule conflict suggestions into an advice
Originally merged to 'next' on 2024-02-27
When a merge conflicted at a submodule, merge-ort backend used to
unconditionally give a lengthy message to suggest how to resolve
it. Now the message can be squelched as an advice message.
Will merge to 'master'.
source: <pull.1661.v2.git.git.1708954048301.gitgitgadget@gmail.com>
* rj/complete-worktree-paths-fix (2024-02-27) 1 commit
- completion: fix __git_complete_worktree_paths
The logic to complete the command line arguments to "git worktree"
subcommand (in contrib/) has been updated to correctly honor things
like "git -C dir" etc.
Comments?
source: <b8f09e20-d0d3-4e0b-afe2-31affeb61052@gmail.com>
* rs/fetch-simplify-with-starts-with (2024-02-26) 1 commit
(merged to 'next' on 2024-03-01 at 000e015fff)
+ fetch: convert strncmp() with strlen() to starts_with()
Originally merged to 'next' on 2024-02-27
Code simplification.
Will merge to 'master'.
source: <cb94b938-03f9-4dd3-84c1-f5244ca81be3@web.de>
* rs/name-rev-with-mempool (2024-02-26) 2 commits
(merged to 'next' on 2024-03-01 at d53eac1836)
+ name-rev: use mem_pool_strfmt()
+ mem-pool: add mem_pool_strfmt()
Originally merged to 'next' on 2024-02-27
Many small allocations "git name-rev" makes have been updated to
allocate from a mem-pool.
Will merge to 'master'.
source: <20240225113947.89357-1-l.s.r@web.de>
* rs/submodule-prefix-simplify (2024-02-26) 1 commit
(merged to 'next' on 2024-03-01 at 05d4d90201)
+ submodule: use strvec_pushf() for --submodule-prefix
Originally merged to 'next' on 2024-02-27
Code simplification.
Will merge to 'master'.
source: <8cd983fb-32b9-41c6-a9e7-a485b190488c@web.de>
* rs/t-ctype-simplify (2024-03-03) 4 commits
(merged to 'next' on 2024-03-04 at 9bd84a8877)
+ t-ctype: avoid duplicating class names
+ t-ctype: align output of i
+ t-ctype: simplify EOF check
+ t-ctype: allow NUL anywhere in the specification string
Code simplification to one unit-test program.
Will merge to 'master'.
source: <20240303101330.20187-1-l.s.r@web.de>
* sg/upload-pack-error-message-fix (2024-02-26) 1 commit
(merged to 'next' on 2024-03-01 at b94664a7a0)
+ upload-pack: don't send null character in abort message to the client
Originally merged to 'next' on 2024-02-27
An error message from "git upload-pack", which responds to "git
fetch" requests, had a trialing NUL in it, which has been
corrected.
Will merge to 'master'.
source: <20240225183452.1939334-1-szeder.dev@gmail.com>
* ak/rebase-autosquash (2024-02-27) 1 commit
(merged to 'next' on 2024-03-01 at 57a34830b7)
+ rebase: fix typo in autosquash documentation
Originally merged to 'next' on 2024-02-27
Typofix.
Will merge to 'master'.
source: <pull.1676.git.1709015578890.gitgitgadget@gmail.com>
* ps/reftable-repo-init-fix (2024-02-27) 2 commits
(merged to 'next' on 2024-03-01 at abbf85051b)
+ refs/reftable: don't fail empty transactions in repo without HEAD
+ Merge branch 'ps/remote-helper-repo-initialization-fix' into ps/reftable-repo-init-fix
(this branch uses ps/remote-helper-repo-initialization-fix.)
Originally merged to 'next' on 2024-02-29
Clear the fallout from a fix for 2.44 regression.
Will merge to 'master'.
source: <95be968e10bd02c64448786e690bbefe5c082577.1709041721.git.ps@pks.im>
* ps/remote-helper-repo-initialization-fix (2024-02-27) 1 commit
(merged to 'next' on 2024-03-01 at 7b79ffbd8f)
+ builtin/clone: allow remote helpers to detect repo
(this branch is used by ps/reftable-repo-init-fix.)
Originally merged to 'next' on 2024-02-29
A custom remote helper no longer cannot access the newly created
repository during "git clone", which is a regression in Git 2.44.
This has been corrected.
Will merge to 'master'.
source: <9d888adf92e9a8af7c18847219f97d3e595e3e36.1709041721.git.ps@pks.im>
* pw/rebase-i-ignore-cherry-pick-help-environment (2024-02-27) 1 commit
- rebase -i: stop setting GIT_CHERRY_PICK_HELP
Code simplification by getting rid of code that sets an environment
variable that is no longer used.
Comments?
source: <pull.1678.git.1709042783847.gitgitgadget@gmail.com>
* as/option-names-in-messages (2024-02-16) 5 commits
- revision.c: trivial fix to message
- builtin/clone.c: trivial fix of message
- builtin/remote.c: trivial fix of error message
- transport-helper.c: trivial fix of error message
- rebase: trivial fix of error message
Error message updates.
Expecting a reroll.
source: <20240216101647.28837-1-ash@kambanaria.org>
* jh/fsmonitor-icase-corner-case-fix (2024-02-26) 14 commits
- fsmonitor: support case-insensitive events
- fsmonitor: refactor bit invalidation in refresh callback
- fsmonitor: trace the new invalidated cache-entry count
- fsmonitor: return invalided cache-entry count on non-directory event
- fsmonitor: remove custom loop from non-directory path handler
- fsmonitor: return invalidated cache-entry count on directory event
- fsmonitor: move untracked-cache invalidation into helper functions
- fsmonitor: refactor untracked-cache invalidation
- dir: create untracked_cache_invalidate_trimmed_path()
- fsmonitor: refactor refresh callback for non-directory events
- fsmonitor: clarify handling of directory events in callback helper
- fsmonitor: refactor refresh callback on directory events
- t7527: add case-insensitve test for FSMonitor
- name-hash: add index_dir_find()
FSMonitor client code was confused when FSEvents were given in a
different case on a case-insensitive filesystem, which has been
corrected.
Comments?
source: <pull.1662.v3.git.1708983565.gitgitgadget@gmail.com>
* ps/reftable-iteration-perf-part2 (2024-03-04) 13 commits
- refs/reftable: precompute prefix length
- reftable: allow inlining of a few functions
- reftable/record: decode keys in place
- reftable/record: reuse refname when copying
- reftable/record: reuse refname when decoding
- reftable/merged: avoid duplicate pqueue emptiness check
- reftable/merged: circumvent pqueue with single subiter
- reftable/merged: handle subiter cleanup on close only
- reftable/merged: remove unnecessary null check for subiters
- reftable/merged: make subiters own their records
- reftable/merged: advance subiter on subsequent iteration
- reftable/merged: make `merged_iter` structure private
- reftable/pq: use `size_t` to track iterator index
The code to iterate over refs with the reftable backend has seen
some optimization.
Will merge to 'next'?
source: <cover.1709548907.git.ps@pks.im>
* js/cmake-with-test-tool (2024-02-23) 2 commits
- cmake: let `test-tool` run the unit tests, too
- Merge branch 'js/unit-test-suite-runner' into js/cmake-with-test-tool
(this branch uses js/unit-test-suite-runner.)
"test-tool" is now built in CMake build to also run the unit tests.
May want to roll it into the base topic.
source: <pull.1666.git.1708038924522.gitgitgadget@gmail.com>
* kn/for-all-refs (2024-02-23) 6 commits
(merged to 'next' on 2024-03-01 at 76a1297ace)
+ for-each-ref: add new option to include root refs
+ ref-filter: rename 'FILTER_REFS_ALL' to 'FILTER_REFS_REGULAR'
+ refs: introduce `refs_for_each_include_root_refs()`
+ refs: extract out `loose_fill_ref_dir_regular_file()`
+ refs: introduce `is_pseudoref()` and `is_headref()`
+ Merge branch 'ps/reftable-backend' into kn/for-all-refs
Originally merged to 'next' on 2024-02-27
"git for-each-ref" learned "--include-root-refs" option to show
even the stuff outside the 'refs/' hierarchy.
Will merge to 'master'.
source: <20240223100112.44127-1-karthik.188@gmail.com>
* jc/no-lazy-fetch (2024-02-27) 3 commits
(merged to 'next' on 2024-03-01 at 14303cdbfe)
+ git: extend --no-lazy-fetch to work across subprocesses
+ git: document GIT_NO_REPLACE_OBJECTS environment variable
+ git: --no-lazy-fetch option
Originally merged to 'next' on 2024-02-29
"git --no-lazy-fetch cmd" allows to run "cmd" while disabling lazy
fetching of objects from the promisor remote, which may be handy
for debugging.
Will merge to 'master'.
source: <xmqq1q8xx38i.fsf@gitster.g>
source: <xmqq1q9cl3xv.fsf@gitster.g>
source: <xmqq1q9mmtpw.fsf@gitster.g>
* js/unit-test-suite-runner (2024-02-23) 8 commits
- ci: use test-tool as unit test runner on Windows
- t/Makefile: run unit tests alongside shell tests
- unit tests: add rule for running with test-tool
- test-tool run-command testsuite: support unit tests
- test-tool run-command testsuite: remove hardcoded filter
- test-tool run-command testsuite: get shell from env
- t0080: turn t-basic unit test into a helper
- Merge branch 'jk/unit-tests-buildfix' into js/unit-test-suite-runner
(this branch is used by js/cmake-with-test-tool.)
The "test-tool" has been taught to run testsuite tests in parallel,
bypassing the need to use the "prove" tool.
Needs review.
source: <cover.1708728717.git.steadmon@google.com>
* cc/rev-list-allow-missing-tips (2024-02-28) 5 commits
(merged to 'next' on 2024-03-01 at fd7b109d04)
+ revision: fix --missing=[print|allow*] for annotated tags
(merged to 'next' on 2024-03-01 at ac0fc94378)
+ rev-list: allow missing tips with --missing=[print|allow*]
+ t6022: fix 'test' style and 'even though' typo
+ oidset: refactor oidset_insert_from_set()
+ revision: clarify a 'return NULL' in get_reference()
Originally merged to 'next' on 2024-02-28
"git rev-list --missing=print" has learned to optionally take
"--allow-missing-tips", which allows the objects at the starting
points to be missing.
Will merge to 'master'.
source: <20240214142513.4002639-1-christian.couder@gmail.com>
* js/merge-tree-3-trees (2024-02-23) 7 commits
(merged to 'next' on 2024-03-01 at a75dc95f04)
+ fill_tree_descriptor(): mark error message for translation
+ cache-tree: avoid an unnecessary check
+ Always check `parse_tree*()`'s return value
+ t4301: verify that merge-tree fails on missing blob objects
+ merge-ort: do check `parse_tree()`'s return value
+ merge-tree: fail with a non-zero exit code on missing tree objects
+ merge-tree: accept 3 trees as arguments
Originally merged to 'next' on 2024-02-28
"git merge-tree" has learned that the three trees involved in the
3-way merge only need to be trees, not necessarily commits.
Will merge to 'master'.
source: <pull.1647.git.1706277694231.gitgitgadget@gmail.com>
source: <pull.1651.v4.git.1708677266.gitgitgadget@gmail.com>
* rj/complete-reflog (2024-03-03) 5 commits
- completion: reflog subcommands and options
- completion: factor out __git_resolve_builtins
- completion: introduce __git_find_subcommand
- completion: reflog show <log-options>
- completion: reflog with implicit "show"
The command line completion script (in contrib/) learned to
complete "git reflog" better.
Will merge to 'next'?
source: <ea6c8890-9ff3-46c9-b933-6a52083b1001@gmail.com>
* ml/log-merge-with-cherry-pick-and-other-pseudo-heads (2024-02-28) 2 commits
(merged to 'next' on 2024-03-01 at 339111ec08)
+ revision: implement `git log --merge` also for rebase/cherry-pick/revert
+ revision: ensure MERGE_HEAD is a ref in prepare_show_merge
Originally merged to 'next' on 2024-02-29
"git log --merge" learned to pay attention to CHERRY_PICK_HEAD and
other kinds of *_HEAD pseudorefs.
Will merge to 'master'.
source: <20240228-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v6-0-8ec34c052b39@gmail.com>
* bk/complete-dirname-for-am-and-format-patch (2024-01-12) 1 commit
- completion: dir-type optargs for am, format-patch
Command line completion support (in contrib/) has been
updated for a few commands to complete directory names where a
directory name is expected.
Expecting a reroll.
cf. <40c3a824-a961-490b-94d4-4eb23c8f713d@gmail.com>
source: <d37781c3-6af2-409b-95a8-660a9b92d20b@smtp-relay.sendinblue.com>
* bk/complete-send-email (2024-01-12) 1 commit
- completion: don't complete revs when --no-format-patch
Command line completion support (in contrib/) has been taught to
avoid offering revision names as candidates to "git send-email" when
the command is used to send pre-generated files.
Expecting a reroll.
cf. <CAC4O8c88Z3ZqxH2VVaNPpEGB3moL5dJcg3cOWuLWwQ_hLrJMtA@mail.gmail.com>
source: <a718b5ee-afb0-44bd-a299-3208fac43506@smtp-relay.sendinblue.com>
* la/trailer-api (2024-03-01) 9 commits
- format_trailers_from_commit(): indirectly call trailer_info_get()
- format_trailer_info(): move "fast path" to caller
- format_trailers(): use strbuf instead of FILE
- trailer_info_get(): reorder parameters
- trailer: move interpret_trailers() to interpret-trailers.c
- trailer: reorder format_trailers_from_commit() parameters
- trailer: rename functions to use 'trailer'
- shortlog: add test for de-duplicating folded trailers
- trailer: free trailer_info _after_ all related usage
source: <pull.1632.v6.git.1709252086.gitgitgadget@gmail.com>
* tb/path-filter-fix (2024-01-31) 16 commits
- bloom: introduce `deinit_bloom_filters()`
- commit-graph: reuse existing Bloom filters where possible
- object.h: fix mis-aligned flag bits table
- commit-graph: new Bloom filter version that fixes murmur3
- commit-graph: unconditionally load Bloom filters
- bloom: prepare to discard incompatible Bloom filters
- bloom: annotate filters with hash version
- repo-settings: introduce commitgraph.changedPathsVersion
- t4216: test changed path filters with high bit paths
- t/helper/test-read-graph: implement `bloom-filters` mode
- bloom.h: make `load_bloom_filter_from_graph()` public
- t/helper/test-read-graph.c: extract `dump_graph_info()`
- gitformat-commit-graph: describe version 2 of BDAT
- commit-graph: ensure Bloom filters are read with consistent settings
- revision.c: consult Bloom filters for root commits
- t/t4216-log-bloom.sh: harden `test_bloom_filters_not_used()`
The Bloom filter used for path limited history traversal was broken
on systems whose "char" is unsigned; update the implementation and
bump the format version to 2.
Waiting for a final ack?
cf. <ZcFjkfbsBfk7JQIH@nand.local>
source: <cover.1706741516.git.me@ttaylorr.com>
* 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.
Will merge to and cook in 'next'?
cf. <xmqqv86z5359.fsf@gitster.g>
source: <878r8l929e.fsf@gmail.froward.int.ebiederm.org>
* jc/rerere-cleanup (2023-08-25) 4 commits
- rerere: modernize use of empty strbuf
- rerere: try_merge() should use LL_MERGE_ERROR when it means an error
- rerere: fix comment on handle_file() helper
- rerere: simplify check_one_conflict() helper function
Code clean-up.
Not ready to be reviewed yet.
source: <20230824205456.1231371-1-gitster@pobox.com>
^ permalink raw reply
* [PATCH] show-ref: add --unresolved option
From: John Cai via GitGitGadget @ 2024-03-04 22:51 UTC (permalink / raw)
To: git; +Cc: John Cai, John Cai
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.
Signed-off-by: John Cai <johncai86@gmail.com>
---
show-ref: add --unresolved option
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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1684%2Fjohn-cai%2Fjc%2Fshow-ref-direct-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1684/john-cai/jc/show-ref-direct-v1
Pull-Request: https://github.com/git/git/pull/1684
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.
+
--abbrev[=<n>]::
Abbreviate the object name. When using `--hash`, you do
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 1c15421e600..58efa078399 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,6 +18,7 @@ static const char * const show_ref_usage[] = {
" [--] [<ref>...]"),
N_("git show-ref --exclude-existing[=<pattern>]"),
N_("git show-ref --exists <ref>"),
+ N_("git show-ref --unresolved <ref>"),
NULL
};
@@ -220,11 +221,11 @@ static int cmd_show_ref__patterns(const struct patterns_options *opts,
return 0;
}
-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));
+
out:
- strbuf_release(&unused_referent);
+ strbuf_release(&referent);
return ret;
}
@@ -284,11 +293,12 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
struct exclude_existing_options exclude_existing_opts = {0};
struct patterns_options patterns_opts = {0};
struct show_one_options show_one_opts = {0};
- int verify = 0, exists = 0;
+ int verify = 0, exists = 0, unresolved = 0;
const struct option show_ref_options[] = {
OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
OPT_BOOL(0, "exists", &exists, N_("check for reference existence without resolving")),
+ OPT_BOOL(0, "unresolved", &unresolved, N_("print out unresolved value of reference")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
@@ -314,16 +324,17 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
- die_for_incompatible_opt3(exclude_existing_opts.enabled, "--exclude-existing",
+ die_for_incompatible_opt4(exclude_existing_opts.enabled, "--exclude-existing",
verify, "--verify",
- exists, "--exists");
+ exists, "--exists",
+ unresolved, "--unresolved");
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
return cmd_show_ref__verify(&show_one_opts, argv);
- else if (exists)
- return cmd_show_ref__exists(argv);
+ else if (exists || unresolved)
+ return cmd_show_ref__raw(argv, unresolved);
else
return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
}
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 33fb7a38fff..11811201738 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -218,6 +218,16 @@ test_expect_success 'show-ref sub-modes are mutually exclusive' '
test_must_fail git show-ref --exclude-existing --exists 2>err &&
grep "exclude-existing" err &&
grep "exists" err &&
+ grep "cannot be used together" err &&
+
+ test_must_fail git show-ref --exclude-existing --unresolved 2>err &&
+ grep "exclude-existing" err &&
+ grep "unresolved" err &&
+ grep "cannot be used together" err &&
+
+ test_must_fail git show-ref --verify --unresolved 2>err &&
+ grep "verify" err &&
+ grep "unresolved" err &&
grep "cannot be used together" err
'
@@ -286,4 +296,41 @@ test_expect_success '--exists with existing special ref' '
git show-ref --exists FETCH_HEAD
'
+test_expect_success '--unresolved with existing reference' '
+ commit_oid=$(git rev-parse refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME) &&
+ cat >expect <<-EOF &&
+ ref: $commit_oid
+ EOF
+ git show-ref --unresolved refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--unresolved with symbolic ref' '
+ test_when_finished "git symbolic-ref -d SYMBOLIC_REF_A" &&
+ cat >expect <<-EOF &&
+ ref: refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ EOF
+ git symbolic-ref SYMBOLIC_REF_A refs/heads/$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+ git show-ref --unresolved SYMBOLIC_REF_A >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--unresolved with nonexistent object ID' '
+ oid=$(test_oid 002) &&
+ test-tool ref-store main update-ref msg refs/heads/missing-oid-2 $oid $ZERO_OID REF_SKIP_OID_VERIFICATION &&
+ cat >expect <<-EOF &&
+ ref: $oid
+ EOF
+ git show-ref --unresolved refs/heads/missing-oid-2 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--unresolved with nonexistent reference' '
+ cat >expect <<-EOF &&
+ error: reference does not exist
+ EOF
+ test_expect_code 2 git show-ref --unresolved refs/heads/not-exist 2>err &&
+ test_cmp expect err
+'
+
test_done
base-commit: b387623c12f3f4a376e4d35a610fd3e55d7ea907
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] show-ref: add --unresolved option
From: Junio C Hamano @ 2024-03-04 23:23 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, John Cai
In-Reply-To: <pull.1684.git.git.1709592718743.gitgitgadget@gmail.com>
"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.
> 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
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.
^ permalink raw reply
* Re: [PATCH v3 2/5] advice: make all entries stylistically consistent
From: Junio C Hamano @ 2024-03-04 23:52 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <d48b4719c275ef06da014b6d22983db9ae484db2.1709590037.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> 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*
> 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
I'll let others comment on these as general principles. I do not
immediately see anything objectionable, but I may change my mind
after reading the updated text in the patch.
> Suggested-by: Junio C Hamano <gitster@pobox.com>
I am getting too much credit for this; I merely suggested to use
"when" instead of "if" in the one you are newly adding.
> 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
> + to move to the detached HEAD state; instruct how to
> create a local branch after the fact.
I agree "Advice shown when" -> "Shown when" is a good change for
brevity, but I do not think the other change is an improvement.
This advice message is shown when the user does X, in order to
instruct the user how to do Y after that. And "to instruct" is a
common way to say the same thing as "in order to instruct".
> 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: tell the user how to
> + set their identity configuration.
Should that be a colon? Stopping a half-sentence and connecting to
another half-sentence is usually done with a semicolon (like you did
in the new version of detachedHEAD above).
Shown when ... and domain name, to tell the user how to set
their identity configuration.
perhaps? There may be other similar entries whose updated text uses
colon followed by an imperative sentence, but I didn't look very
carefully.
> 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: consider using the
> + `-u` option.
Earlier ones after a colon (or semicolon in detachedHEAD case), you
gave an order to the advice message (e.g. "hey detachedHead advice,
tell the user how to create a local branch"), but this one is giving
an order to the end user, which feels inconsistent.
I do not have a strong objection against giving an order to the
advice message, as long as it is done consistently. If we did so,
the part after the colon would start with "instruct the user ..." or
"tell the user ..." and the like, and the gist of what this one
would say would be "shown when it is taking too long: suggest the
user to consider `-u`".
FWIW, my earlier "in order to" took an approach that is different
from either of the two "giving an order" approaches. I was trying
to make the description explain what the message tries to do and/or
why the message is given (e.g., "shown if it takes too long in order
to suggest users to consider the -u option").
Thanks.
^ permalink raw reply
* Re: [GSOC][PATCH v3 1/1] t7301: use test_path_is_(missing|file)
From: Eric Sunshine @ 2024-03-04 23:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Vincenzo Mezzela, git
In-Reply-To: <xmqqcys9oedq.fsf@gitster.g>
On Mon, Mar 4, 2024 at 4:51 PM Junio C Hamano <gitster@pobox.com> wrote:
> Vincenzo Mezzela <vincenzo.mezzela@gmail.com> writes:
> > Refactor test -f to utilize the corresponding helper functions from
>
> As Eric pointed out to another GSoC applicant in a different thread,
> this is not refactoring any code, so saying "refactor" is a bit
> misleading.
This is probably a reference to [1], though I was definitely thinking
of [2] when I wrote [1], so we come full-circle.
[1]: https://lore.kernel.org/git/CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com/
[2]: https://lore.kernel.org/git/xmqqmst2nszq.fsf@gitster.g/
^ permalink raw reply
* Re: [PATCH v3 3/5] advice: use backticks for code
From: Junio C Hamano @ 2024-03-04 23:54 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <30d662a04c75b80166db9ef94f95e8a841994fb5.1709590037.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> Use backticks for quoting code rather than single quotes.
Good. Technically it does not have to be "code", but rather what
the user would literally type from their keyboard verbatim, but
"quoting code" is so concise way to describe, it probably is good
enough hint for future developers who will find this commit via "git
blame" and read "git show" to read this explanation.
> Also replace “the add command” with “`git add`”.
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> Documentation/config/advice.txt | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index cfca87a6aa2..df447dd5d14 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::
> @@ -90,9 +90,9 @@ advice.*::
> 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
^ permalink raw reply
* [RFC PATCH 0/3] Parallel submodule status
From: Atneya Nair @ 2024-03-05 1:21 UTC (permalink / raw)
To: git; +Cc: gitster, jeffhost, me, nasamuffin, Atneya Nair
This set tries to speed up git status (and other commands which examine
the tree state) for repositories with many submodules.
Currently, submodules, unlike regular files, are examined serially
despite the expense of this operation. Fsmonitor also doesn't support
submodules, so there isn't a great alternative here.
Atneya Nair (3):
Make read_gitfile and resolve_gitfile thread safe
Make ce_compare_gitlink thread-safe
Preload submodule state in refresh_index
builtin/init-db.c | 7 ++++---
builtin/rev-parse.c | 4 +++-
config.c | 3 ++-
config.h | 2 +-
preload-index.c | 25 ++++++++++++++++++++++---
read-cache-ll.h | 1 +
read-cache.c | 3 +++
refs.c | 9 +++++++++
repository.c | 9 +++++----
setup.c | 36 +++++++++++++++++++++++++-----------
setup.h | 7 +++----
submodule.c | 32 +++++++++++++++++++++++---------
worktree.c | 27 +++++++++++++--------------
13 files changed, 114 insertions(+), 51 deletions(-)
base-commit: f41f85c9ec8d4d46de0fd5fded88db94d3ec8c11
--
2.44.0.rc1.240.g4c46232300-goog
^ permalink raw reply
* [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe
From: Atneya Nair @ 2024-03-05 1:21 UTC (permalink / raw)
To: git; +Cc: gitster, jeffhost, me, nasamuffin, Atneya Nair
In-Reply-To: <20240305012112.1598053-2-atneya@google.com>
Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff
to remove unsafe shared buffer usage in read_gitfile_gently.
Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf
out params to allocate their return values into, rather than returning
a view into a shared buffer.
Leave the shared buffer in case a caller passes null for this param (for
cases we haven't cleaned up yet).
Migrate callers of resolve_gitfile to resolve_gitfile_gently.
Signed-off-by: Atneya Nair <atneya@google.com>
---
Notes:
Open questions:
- Is checking the return value of read_gitfile necessary if on error,
we are supposed to die, or set the error field to a non-zero value?
- Should we clean up the other call-sites of read_gitfile?
builtin/init-db.c | 7 ++++---
builtin/rev-parse.c | 4 +++-
repository.c | 9 +++++----
setup.c | 36 +++++++++++++++++++++++++-----------
setup.h | 7 +++----
submodule.c | 32 +++++++++++++++++++++++---------
worktree.c | 27 +++++++++++++--------------
7 files changed, 76 insertions(+), 46 deletions(-)
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0170469b84..9135d07a0d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
*/
if (real_git_dir) {
int err;
- const char *p;
struct strbuf sb = STRBUF_INIT;
+ struct strbuf gitfile = STRBUF_INIT;
- p = read_gitfile_gently(git_dir, &err);
- if (p && get_common_dir(&sb, p)) {
+ read_gitfile_gently(git_dir, &err, &gitfile);
+ if (!err && get_common_dir(&sb, gitfile.buf)) {
struct strbuf mainwt = STRBUF_INIT;
strbuf_addbuf(&mainwt, &sb);
@@ -213,6 +213,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
git_dir = strbuf_detach(&sb, NULL);
}
strbuf_release(&sb);
+ strbuf_release(&gitfile);
}
if (is_bare_repository_cfg < 0)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index d08987646a..e1db6b3231 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -728,12 +728,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
}
if (!strcmp(arg, "--resolve-git-dir")) {
const char *gitdir = argv[++i];
+ struct strbuf resolved_gitdir = STRBUF_INIT;
if (!gitdir)
die(_("--resolve-git-dir requires an argument"));
- gitdir = resolve_gitdir(gitdir);
+ gitdir = resolve_gitdir_gently(gitdir, NULL, &resolved_gitdir);
if (!gitdir)
die(_("not a gitdir '%s'"), argv[i]);
puts(gitdir);
+ strbuf_release(&resolved_gitdir);
continue;
}
}
diff --git a/repository.c b/repository.c
index 7aacb51b65..3ca6dbcf16 100644
--- a/repository.c
+++ b/repository.c
@@ -118,7 +118,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
int ret = 0;
int error = 0;
char *abspath = NULL;
- const char *resolved_gitdir;
+ struct strbuf resolved_gitdir = STRBUF_INIT;
struct set_gitdir_args args = { NULL };
abspath = real_pathdup(gitdir, 0);
@@ -128,15 +128,16 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
}
/* 'gitdir' must reference the gitdir directly */
- resolved_gitdir = resolve_gitdir_gently(abspath, &error);
- if (!resolved_gitdir) {
+ resolve_gitdir_gently(abspath, &error, &resolved_gitdir);
+ if (error) {
ret = -1;
goto out;
}
- repo_set_gitdir(repo, resolved_gitdir, &args);
+ repo_set_gitdir(repo, resolved_gitdir.buf, &args);
out:
+ strbuf_release(&resolved_gitdir);
free(abspath);
return ret;
}
diff --git a/setup.c b/setup.c
index b69b1cbc2a..2e118cf216 100644
--- a/setup.c
+++ b/setup.c
@@ -397,14 +397,17 @@ int is_nonbare_repository_dir(struct strbuf *path)
int ret = 0;
int gitfile_error;
size_t orig_path_len = path->len;
+ struct strbuf gitfile = STRBUF_INIT;
+
assert(orig_path_len != 0);
strbuf_complete(path, '/');
strbuf_addstr(path, ".git");
- if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
+ if (read_gitfile_gently(path->buf, &gitfile_error, &gitfile) || is_git_directory(path->buf))
ret = 1;
if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
gitfile_error == READ_GITFILE_ERR_READ_FAILED)
ret = 1;
+ strbuf_release(&gitfile);
strbuf_setlen(path, orig_path_len);
return ret;
}
@@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
/*
* Try to read the location of the git directory from the .git file,
- * return path to git directory if found. The return value comes from
+ * return path to git directory if found. If passed a valid strbuf, the return
+ * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
* a shared buffer.
*
* On failure, if return_error_code is not NULL, return_error_code
@@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
* return_error_code is NULL the function will die instead (for most
* cases).
*/
-const char *read_gitfile_gently(const char *path, int *return_error_code)
+const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
{
const int max_file_size = 1 << 20; /* 1MB */
int error_code = 0;
@@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
struct stat st;
int fd;
ssize_t len;
- static struct strbuf realpath = STRBUF_INIT;
+ static struct strbuf shared = STRBUF_INIT;
+ if (!result_buf) {
+ result_buf = &shared;
+ }
if (stat(path, &st)) {
/* NEEDSWORK: discern between ENOENT vs other errors */
@@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
goto cleanup_return;
}
- strbuf_realpath(&realpath, dir, 1);
- path = realpath.buf;
+ strbuf_realpath(result_buf, dir, 1);
+ path = result_buf->buf;
+ // TODO is this valid?
+ if (!path) die(_("Unexpected null from realpath '%s'"), dir);
cleanup_return:
if (return_error_code)
@@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
int offset = dir->len, error_code = 0;
char *gitdir_path = NULL;
char *gitfile = NULL;
+ struct strbuf gitdirenvbuf = STRBUF_INIT;
if (offset > min_offset)
strbuf_addch(dir, '/');
strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
- NULL : &error_code);
+ NULL : &error_code, &gitdirenvbuf);
if (!gitdirenv) {
if (die_on_error ||
error_code == READ_GITFILE_ERR_NOT_A_FILE) {
@@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
gitdir_path = xstrdup(dir->buf);
}
- } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
+ } else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
+ strbuf_release(&gitdirenvbuf);
return GIT_DIR_INVALID_GITFILE;
+ }
} else
gitfile = xstrdup(dir->buf);
/*
@@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
*/
free(gitdir_path);
free(gitfile);
-
+ strbuf_release(&gitdirenvbuf);
return ret;
}
+ strbuf_release(&gitdirenvbuf);
if (is_git_directory(dir->buf)) {
trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
@@ -1692,11 +1705,12 @@ const char *setup_git_directory(void)
return setup_git_directory_gently(NULL);
}
-const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code,
+ struct strbuf* return_buf)
{
if (is_git_directory(suspect))
return suspect;
- return read_gitfile_gently(suspect, return_error_code);
+ return read_gitfile_gently(suspect, return_error_code, return_buf);
}
/* if any standard file descriptor is missing open it to /dev/null */
diff --git a/setup.h b/setup.h
index 3599aec93c..cf5a63762b 100644
--- a/setup.h
+++ b/setup.h
@@ -36,10 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);
#define READ_GITFILE_ERR_NOT_A_REPO 7
#define READ_GITFILE_ERR_TOO_LARGE 8
void read_gitfile_error_die(int error_code, const char *path, const char *dir);
-const char *read_gitfile_gently(const char *path, int *return_error_code);
-#define read_gitfile(path) read_gitfile_gently((path), NULL)
-const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
-#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
+const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf *buf);
+#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)
+const char *resolve_gitdir_gently(const char *suspect, int *return_error_code, struct strbuf *buf);
void setup_work_tree(void);
diff --git a/submodule.c b/submodule.c
index 213da79f66..455444321b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
int ret = 0;
char *gitdir = xstrfmt("%s/.git", path);
- if (resolve_gitdir_gently(gitdir, return_error_code))
+ struct strbuf resolved_gitdir_buf = STRBUF_INIT;
+ if (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))
ret = 1;
-
+ strbuf_release(&resolved_gitdir_buf);
free(gitdir);
return ret;
}
@@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
{
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
+ struct strbuf gitdirbuf = STRBUF_INIT;
FILE *fp;
unsigned dirty_submodule = 0;
const char *git_dir;
int ignore_cp_exit_code = 0;
strbuf_addf(&buf, "%s/.git", path);
- git_dir = read_gitfile(buf.buf);
+ git_dir = read_gitfile_gently(buf.buf, NULL, &gitdirbuf);
if (!git_dir)
git_dir = buf.buf;
if (!is_git_directory(git_dir)) {
if (is_directory(git_dir))
die(_("'%s' not recognized as a git repository"), git_dir);
strbuf_release(&buf);
+ strbuf_release(&gitdirbuf);
/* The submodule is not checked out, so it is not modified */
return 0;
}
+ strbuf_release(&gitdirbuf);
strbuf_reset(&buf);
strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
@@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)
{
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
- const char *git_dir;
+ struct strbuf gitfilebuf = STRBUF_INIT;
strbuf_addf(&buf, "%s/.git", path);
- git_dir = read_gitfile(buf.buf);
- if (!git_dir) {
+ read_gitfile_gently(buf.buf, NULL, &gitfilebuf);
+ if (!gitfilebuf.buf) {
strbuf_release(&buf);
return 0;
}
strbuf_release(&buf);
+ strbuf_release(&gitfilebuf);
/* Now test that all nested submodules use a gitfile too */
strvec_pushl(&cp.args,
@@ -2276,6 +2281,7 @@ static void relocate_single_git_dir_into_superproject(const char *path,
{
char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
struct strbuf new_gitdir = STRBUF_INIT;
+ struct strbuf gitfilebuf = STRBUF_INIT;
const struct submodule *sub;
if (submodule_uses_worktrees(path))
@@ -2283,9 +2289,12 @@ static void relocate_single_git_dir_into_superproject(const char *path,
"more than one worktree not supported"), path);
old_git_dir = xstrfmt("%s/.git", path);
- if (read_gitfile(old_git_dir))
+ if (read_gitfile_gently(old_git_dir, NULL, &gitfilebuf)) {
/* If it is an actual gitfile, it doesn't need migration. */
+ strbuf_release(&gitfilebuf);
return;
+ }
+ strbuf_release(&gitfilebuf);
real_old_git_dir = real_pathdup(old_git_dir, 1);
@@ -2343,8 +2352,9 @@ void absorb_git_dir_into_superproject(const char *path,
int err_code;
const char *sub_git_dir;
struct strbuf gitdir = STRBUF_INIT;
+ struct strbuf resolved_gitdir_buf = STRBUF_INIT;
strbuf_addf(&gitdir, "%s/.git", path);
- sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
+ sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code, &resolved_gitdir_buf);
/* Not populated? */
if (!sub_git_dir) {
@@ -2385,6 +2395,8 @@ void absorb_git_dir_into_superproject(const char *path,
free(real_sub_git_dir);
free(real_common_git_dir);
}
+
+ strbuf_release(&resolved_gitdir_buf);
strbuf_release(&gitdir);
absorb_git_dir_into_superproject_recurse(path, super_prefix);
@@ -2484,17 +2496,19 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
const struct submodule *sub;
const char *git_dir;
int ret = 0;
+ struct strbuf gitfilebuf = STRBUF_INIT;
strbuf_reset(buf);
strbuf_addstr(buf, submodule);
strbuf_complete(buf, '/');
strbuf_addstr(buf, ".git");
- git_dir = read_gitfile(buf->buf);
+ git_dir = read_gitfile_gently(buf->buf, NULL, &gitfilebuf);
if (git_dir) {
strbuf_reset(buf);
strbuf_addstr(buf, git_dir);
}
+ strbuf_release(&gitfilebuf);
if (!is_git_directory(buf->buf)) {
sub = submodule_from_path(the_repository, null_oid(),
submodule);
diff --git a/worktree.c b/worktree.c
index b02a05a74a..a6f125c8da 100644
--- a/worktree.c
+++ b/worktree.c
@@ -309,7 +309,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
{
struct strbuf wt_path = STRBUF_INIT;
struct strbuf realpath = STRBUF_INIT;
- char *path = NULL;
+ struct strbuf gitfile = STRBUF_INIT;
int err, ret = -1;
strbuf_addf(&wt_path, "%s/.git", wt->path);
@@ -353,21 +353,20 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
goto done;
}
- path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
- if (!path) {
+ if (!read_gitfile_gently(wt_path.buf, &err, &gitfile)) {
strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
wt_path.buf, err);
goto done;
}
strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);
- ret = fspathcmp(path, realpath.buf);
+ ret = fspathcmp(gitfile.buf, realpath.buf);
if (ret)
strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
wt->path, git_common_path("worktrees/%s", wt->id));
done:
- free(path);
+ strbuf_release(&gitfile);
strbuf_release(&wt_path);
strbuf_release(&realpath);
return ret;
@@ -567,7 +566,7 @@ static void repair_gitfile(struct worktree *wt,
{
struct strbuf dotgit = STRBUF_INIT;
struct strbuf repo = STRBUF_INIT;
- char *backlink;
+ struct strbuf backlink = STRBUF_INIT;
const char *repair = NULL;
int err;
@@ -582,13 +581,13 @@ static void repair_gitfile(struct worktree *wt,
strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
strbuf_addf(&dotgit, "%s/.git", wt->path);
- backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+ read_gitfile_gently(dotgit.buf, &err, &backlink);
if (err == READ_GITFILE_ERR_NOT_A_FILE)
fn(1, wt->path, _(".git is not a file"), cb_data);
else if (err)
repair = _(".git file broken");
- else if (fspathcmp(backlink, repo.buf))
+ else if (fspathcmp(backlink.buf, repo.buf))
repair = _(".git file incorrect");
if (repair) {
@@ -596,7 +595,7 @@ static void repair_gitfile(struct worktree *wt,
write_file(dotgit.buf, "gitdir: %s", repo.buf);
}
- free(backlink);
+ strbuf_release(&backlink);
strbuf_release(&repo);
strbuf_release(&dotgit);
}
@@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,
struct strbuf realdotgit = STRBUF_INIT;
struct strbuf gitdir = STRBUF_INIT;
struct strbuf olddotgit = STRBUF_INIT;
- char *backlink = NULL;
+ struct strbuf backlink = STRBUF_INIT;
const char *repair = NULL;
int err;
@@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,
goto done;
}
- backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
+ read_gitfile_gently(realdotgit.buf, &err, &backlink);
if (err == READ_GITFILE_ERR_NOT_A_FILE) {
fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
goto done;
} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
- if (!(backlink = infer_backlink(realdotgit.buf))) {
+ if (!(backlink.buf = infer_backlink(realdotgit.buf))) {
fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
goto done;
}
@@ -715,7 +714,7 @@ void repair_worktree_at_path(const char *path,
goto done;
}
- strbuf_addf(&gitdir, "%s/gitdir", backlink);
+ strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
repair = _("gitdir unreadable");
else {
@@ -729,7 +728,7 @@ void repair_worktree_at_path(const char *path,
write_file(gitdir.buf, "%s", realdotgit.buf);
}
done:
- free(backlink);
+ strbuf_release(&backlink);
strbuf_release(&olddotgit);
strbuf_release(&gitdir);
strbuf_release(&realdotgit);
--
2.44.0.rc1.240.g4c46232300-goog
^ permalink raw reply related
* [RFC PATCH 2/3] Make ce_compare_gitlink thread-safe
From: Atneya Nair @ 2024-03-05 1:21 UTC (permalink / raw)
To: git; +Cc: gitster, jeffhost, me, nasamuffin, Atneya Nair
In-Reply-To: <20240305012112.1598053-2-atneya@google.com>
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.
Signed-off-by: Atneya Nair <atneya@google.com>
---
Notes:
Chasing down thread unsafe code was done using tsan.
Open questions:
- Is there any additional thread-unsafety that was missed?
- Is it safe to strdup in do_config_from (do the filenames need static
lifetime)? What is the performance implication (it seems small)?
- Is the locking around ref_store_map appropriate and/or can it be made
more granular?
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);
--
2.44.0.rc1.240.g4c46232300-goog
^ permalink raw reply related
* [RFC PATCH 3/3] Preload submodule state in refresh_index
From: Atneya Nair @ 2024-03-05 1:21 UTC (permalink / raw)
To: git; +Cc: gitster, jeffhost, me, nasamuffin, Atneya Nair
In-Reply-To: <20240305012112.1598053-2-atneya@google.com>
refresh_index currently parallelizes updating cache_entries for regular
files based on lstat. Expand preload to parallelize exploring the
checked out state of submodules, which is substantially more expensive.
Cache the state of the submodule in memory, to avoid unnecessary
re-computation (similar to regular files).
This speeds up git status, and other operations which examine the read
index, especially in repositories with many submodules.
Signed-off-by: Atneya Nair <atneya@google.com>
---
Notes:
For now, I added a new field to store the submodule state.
Open questions:
- Where can we efficiently store the submodule state? I assume we can
re-use some of the ce_flags which aren't used for submodules?
- Why can threads only go up to 64? Can we make this user configurable?
preload-index.c | 25 ++++++++++++++++++++++---
read-cache-ll.h | 1 +
read-cache.c | 3 +++
3 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/preload-index.c b/preload-index.c
index 63fd35d64b..091b22fa4c 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -22,7 +22,7 @@
* to have at least 500 lstat's per thread for it to
* be worth starting a thread.
*/
-#define MAX_PARALLEL (20)
+#define MAX_PARALLEL (60)
#define THREAD_COST (500)
struct progress_data {
@@ -59,8 +59,21 @@ static void *preload_thread(void *_data)
if (ce_stage(ce))
continue;
- if (S_ISGITLINK(ce->ce_mode))
+ if (S_ISGITLINK(ce->ce_mode)) {
+ // This call evaluates the submodule HEAD for GITLINK, which really does determine
+ // if there is a change (for index purposes). We can't use the traditional path of
+ // marking as VALID, because valid can't be used for submodules due to other code
+ // paths in which valid may skip investigation of the worktree in the submodule.
+ // Gitlinks also aren't statable, or fsmonitorable, so caching doesn't have the same
+ // semantics.
+ // Use a special entry to mark the ref change state and its validity. Future calls
+ // to ce_compare_gitlink will leverage this.
+ if (lstat(ce->name, &st))
+ continue;
+ ce->sub_ref_state = (!!(ie_match_stat(index, ce, &st,
+ CE_MATCH_RACY_IS_DIRTY|CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED) << 1) | 0x1;
continue;
+ }
if (ce_uptodate(ce))
continue;
if (ce_skip_worktree(ce))
@@ -107,11 +120,17 @@ void preload_index(struct index_state *index,
struct thread_data data[MAX_PARALLEL];
struct progress_data pd;
int t2_sum_lstat = 0;
+ int link_count = 0;
if (!HAVE_THREADS || !core_preload_index)
return;
- threads = index->cache_nr / THREAD_COST;
+ for (i = 0; i < index->cache_nr; i++) {
+ link_count += (S_ISGITLINK(index->cache[i]->ce_mode));
+ }
+ // Exploring gitlinks are much more expensive than lstat, so modify the cost
+ threads = (index->cache_nr / THREAD_COST) + (link_count / 25);
+
if ((index->cache_nr > 1) && (threads < 2) && git_env_bool("GIT_TEST_PRELOAD_INDEX", 0))
threads = 2;
if (threads < 2)
diff --git a/read-cache-ll.h b/read-cache-ll.h
index 2a50a784f0..5555bb0ae9 100644
--- a/read-cache-ll.h
+++ b/read-cache-ll.h
@@ -27,6 +27,7 @@ struct cache_entry {
unsigned int mem_pool_allocated;
unsigned int ce_namelen;
unsigned int index; /* for link extension */
+ unsigned int sub_ref_state; /* TODO pack somewhere. Lowest bit valid, second lowest dirty. */
struct object_id oid;
char name[FLEX_ARRAY]; /* more */
};
diff --git a/read-cache.c b/read-cache.c
index f546cf7875..541d40ca30 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -271,6 +271,9 @@ static int ce_compare_gitlink(const struct cache_entry *ce)
*
* If so, we consider it always to match.
*/
+ if (ce->sub_ref_state & 0x1)
+ /* Check the cached value */
+ return ce->sub_ref_state >> 1;
if (resolve_gitlink_ref(ce->name, "HEAD", &oid) < 0)
return 0;
return !oideq(&oid, &ce->oid);
--
2.44.0.rc1.240.g4c46232300-goog
^ permalink raw reply related
* Re: [PATCH v3 1/5] t3200: improve test style
From: Junio C Hamano @ 2024-03-05 1:25 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <e6a2628ce57668aa17101e73edaead0ef34d8a8c.1709590037.git.code@khaugsbakk.name>
Kristoffer Haugsbakk <code@khaugsbakk.name> writes:
> 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
> • Move a `git config` command into the test and mark it as `setup` since
> the next test depends on it
>
Especially the change to use "-\EOF" to make them align better
caused too many tests to be touched, but overall the result may have
become much easier to follow. Good job.
> -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
The above is a truly valuable clean-up.
But I am not really sure if the paritcular condition is worth
testing in the first place these days. No configuration file means
we cannot even read the repository format version, and working under
such a condition is quite a bad promise that we would rather not to
having to keep. But that is an entirely different topic from what
this patch is doing.
> -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' '
> + 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 &&
I do not know if the change of the title is warranted. It is doing
its own test, not just setup. It may be merely donw for the side
effect of making the step unskippable, but still ....
> -# Keep this test last, as it changes the current branch
Yes, removal of this line is really appreciated ;-)
> -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
> '
Thanks.
^ permalink raw reply
* [PATCH] gpg-interface: Look up signing keys with only email address
From: Marco Sirabella @ 2024-03-05 2:08 UTC (permalink / raw)
To: git; +Cc: Marco Sirabella
Sometimes gpg signing key UIDs include a comment between the name and
parenthesis in the form of:
John Smith (example) jsmith@example.com
There's no way for git to find signing keys associated with just these
UIDs, so look up a partial match on the email only.
Signed-off-by: Marco Sirabella <marco@sirabella.org>
---
gpg-interface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 95e764acb1..3c303e05bd 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -922,7 +922,7 @@ const char *get_signing_key(void)
return use_format->get_default_key();
}
- return git_committer_info(IDENT_STRICT | IDENT_NO_DATE);
+ return git_committer_info(IDENT_NO_NAME | IDENT_STRICT | IDENT_NO_DATE);
}
const char *gpg_trust_level_to_str(enum signature_trust_level level)
--
2.43.0
^ permalink raw reply related
* Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe
From: Junio C Hamano @ 2024-03-05 2:22 UTC (permalink / raw)
To: Atneya Nair; +Cc: git, jeffhost, me, nasamuffin
In-Reply-To: <20240305012112.1598053-3-atneya@google.com>
Atneya Nair <atneya@google.com> writes:
> Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff
> to remove unsafe shared buffer usage in read_gitfile_gently.
The usual way to compose a log message of this project is to
- Give an observation on how the current system work in the present
tense (so no need to say "Currently X is Y", just "X is Y"), and
discuss what you perceive as a problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to the codebase to "become like so".
in this order. The introductory part may become like so. Notice
the format in which we usually refer to an existing commit:
3d7747e3 (real_path: remove unsafe API, 2020-03-10) started the
process of making more API functions thread-safe. Many callers
of read_gitfile() still depend on the shared buffer used to hold
the return value, and convenience of not having to allocate/free
their own storage, but this hinders multi-threading.
And the remainder of the proposed log message should propose a
solution and tell the codebase to become like so.
See Documentation/SubmittingPatches for other general guidelines.
Documentation/CodingGuidelines would also be helpful.
> Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf
> out params to allocate their return values into, rather than returning
> a view into a shared buffer.
>
> Leave the shared buffer in case a caller passes null for this param (for
> cases we haven't cleaned up yet).
>
> Migrate callers of resolve_gitfile to resolve_gitfile_gently.
>
> Signed-off-by: Atneya Nair <atneya@google.com>
> ---
>
> Notes:
> Open questions:
> - Is checking the return value of read_gitfile necessary if on error,
> we are supposed to die, or set the error field to a non-zero value?
The conversion done by this step seems to be a more or less faithful
rewrite. If the original checked for an error from the original
read_gitfile_gently() by seeing if it returned a NULL, the updated
code should instead check for what is returned in &err. And that
seems to be what you did, which is good.
> - Should we clean up the other call-sites of read_gitfile?
It is OK to do the ones you care about (read: needed to be converted
before you can do the multi-threading) first, and then update the
rest.
The conversion itself in the patch looked obvious and trivially
correct from a cursory read, but I have to admit that my
concentration level was not sufficiently high while I reviewed the
patch, as my reading was constantly hiccupping whenever I saw a
coding style glitches. The authors can help by sticking to what the
Documentation/CodingGuidelines document says, paying special
attention to the comment styles, decl-after-statement, rules
regarding braces around a single-statement block.
Thanks.
> builtin/init-db.c | 7 ++++---
> builtin/rev-parse.c | 4 +++-
> repository.c | 9 +++++----
> setup.c | 36 +++++++++++++++++++++++++-----------
> setup.h | 7 +++----
> submodule.c | 32 +++++++++++++++++++++++---------
> worktree.c | 27 +++++++++++++--------------
> 7 files changed, 76 insertions(+), 46 deletions(-)
>
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 0170469b84..9135d07a0d 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> */
> if (real_git_dir) {
> int err;
> - const char *p;
> struct strbuf sb = STRBUF_INIT;
> + struct strbuf gitfile = STRBUF_INIT;
>
> - p = read_gitfile_gently(git_dir, &err);
> - if (p && get_common_dir(&sb, p)) {
> + read_gitfile_gently(git_dir, &err, &gitfile);
> + if (!err && get_common_dir(&sb, gitfile.buf)) {
> struct strbuf mainwt = STRBUF_INIT;
>
> strbuf_addbuf(&mainwt, &sb);
> @@ -213,6 +213,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> git_dir = strbuf_detach(&sb, NULL);
> }
> strbuf_release(&sb);
> + strbuf_release(&gitfile);
> }
>
> if (is_bare_repository_cfg < 0)
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index d08987646a..e1db6b3231 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -728,12 +728,14 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> }
> if (!strcmp(arg, "--resolve-git-dir")) {
> const char *gitdir = argv[++i];
> + struct strbuf resolved_gitdir = STRBUF_INIT;
> if (!gitdir)
> die(_("--resolve-git-dir requires an argument"));
> - gitdir = resolve_gitdir(gitdir);
> + gitdir = resolve_gitdir_gently(gitdir, NULL, &resolved_gitdir);
> if (!gitdir)
> die(_("not a gitdir '%s'"), argv[i]);
> puts(gitdir);
> + strbuf_release(&resolved_gitdir);
> continue;
> }
> }
> diff --git a/repository.c b/repository.c
> index 7aacb51b65..3ca6dbcf16 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -118,7 +118,7 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
> int ret = 0;
> int error = 0;
> char *abspath = NULL;
> - const char *resolved_gitdir;
> + struct strbuf resolved_gitdir = STRBUF_INIT;
> struct set_gitdir_args args = { NULL };
>
> abspath = real_pathdup(gitdir, 0);
> @@ -128,15 +128,16 @@ static int repo_init_gitdir(struct repository *repo, const char *gitdir)
> }
>
> /* 'gitdir' must reference the gitdir directly */
> - resolved_gitdir = resolve_gitdir_gently(abspath, &error);
> - if (!resolved_gitdir) {
> + resolve_gitdir_gently(abspath, &error, &resolved_gitdir);
> + if (error) {
> ret = -1;
> goto out;
> }
>
> - repo_set_gitdir(repo, resolved_gitdir, &args);
> + repo_set_gitdir(repo, resolved_gitdir.buf, &args);
>
> out:
> + strbuf_release(&resolved_gitdir);
> free(abspath);
> return ret;
> }
> diff --git a/setup.c b/setup.c
> index b69b1cbc2a..2e118cf216 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -397,14 +397,17 @@ int is_nonbare_repository_dir(struct strbuf *path)
> int ret = 0;
> int gitfile_error;
> size_t orig_path_len = path->len;
> + struct strbuf gitfile = STRBUF_INIT;
> +
> assert(orig_path_len != 0);
> strbuf_complete(path, '/');
> strbuf_addstr(path, ".git");
> - if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
> + if (read_gitfile_gently(path->buf, &gitfile_error, &gitfile) || is_git_directory(path->buf))
> ret = 1;
> if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
> gitfile_error == READ_GITFILE_ERR_READ_FAILED)
> ret = 1;
> + strbuf_release(&gitfile);
> strbuf_setlen(path, orig_path_len);
> return ret;
> }
> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>
> /*
> * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found. The return value comes from
> + * return path to git directory if found. If passed a valid strbuf, the return
> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
> * a shared buffer.
> *
> * On failure, if return_error_code is not NULL, return_error_code
> @@ -838,7 +842,7 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
> * return_error_code is NULL the function will die instead (for most
> * cases).
> */
> -const char *read_gitfile_gently(const char *path, int *return_error_code)
> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf* result_buf)
> {
> const int max_file_size = 1 << 20; /* 1MB */
> int error_code = 0;
> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> struct stat st;
> int fd;
> ssize_t len;
> - static struct strbuf realpath = STRBUF_INIT;
> + static struct strbuf shared = STRBUF_INIT;
> + if (!result_buf) {
> + result_buf = &shared;
> + }
>
> if (stat(path, &st)) {
> /* NEEDSWORK: discern between ENOENT vs other errors */
> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> goto cleanup_return;
> }
>
> - strbuf_realpath(&realpath, dir, 1);
> - path = realpath.buf;
> + strbuf_realpath(result_buf, dir, 1);
> + path = result_buf->buf;
> + // TODO is this valid?
> + if (!path) die(_("Unexpected null from realpath '%s'"), dir);
>
> cleanup_return:
> if (return_error_code)
> @@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> int offset = dir->len, error_code = 0;
> char *gitdir_path = NULL;
> char *gitfile = NULL;
> + struct strbuf gitdirenvbuf = STRBUF_INIT;
>
> if (offset > min_offset)
> strbuf_addch(dir, '/');
> strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
> gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
> - NULL : &error_code);
> + NULL : &error_code, &gitdirenvbuf);
> if (!gitdirenv) {
> if (die_on_error ||
> error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> @@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
> gitdir_path = xstrdup(dir->buf);
> }
> - } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
> + } else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
> + strbuf_release(&gitdirenvbuf);
> return GIT_DIR_INVALID_GITFILE;
> + }
> } else
> gitfile = xstrdup(dir->buf);
> /*
> @@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> */
> free(gitdir_path);
> free(gitfile);
> -
> + strbuf_release(&gitdirenvbuf);
> return ret;
> }
> + strbuf_release(&gitdirenvbuf);
>
> if (is_git_directory(dir->buf)) {
> trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
> @@ -1692,11 +1705,12 @@ const char *setup_git_directory(void)
> return setup_git_directory_gently(NULL);
> }
>
> -const char *resolve_gitdir_gently(const char *suspect, int *return_error_code)
> +const char *resolve_gitdir_gently(const char *suspect, int *return_error_code,
> + struct strbuf* return_buf)
> {
> if (is_git_directory(suspect))
> return suspect;
> - return read_gitfile_gently(suspect, return_error_code);
> + return read_gitfile_gently(suspect, return_error_code, return_buf);
> }
>
> /* if any standard file descriptor is missing open it to /dev/null */
> diff --git a/setup.h b/setup.h
> index 3599aec93c..cf5a63762b 100644
> --- a/setup.h
> +++ b/setup.h
> @@ -36,10 +36,9 @@ int is_nonbare_repository_dir(struct strbuf *path);
> #define READ_GITFILE_ERR_NOT_A_REPO 7
> #define READ_GITFILE_ERR_TOO_LARGE 8
> void read_gitfile_error_die(int error_code, const char *path, const char *dir);
> -const char *read_gitfile_gently(const char *path, int *return_error_code);
> -#define read_gitfile(path) read_gitfile_gently((path), NULL)
> -const char *resolve_gitdir_gently(const char *suspect, int *return_error_code);
> -#define resolve_gitdir(path) resolve_gitdir_gently((path), NULL)
> +const char *read_gitfile_gently(const char *path, int *return_error_code, struct strbuf *buf);
> +#define read_gitfile(path) read_gitfile_gently((path), NULL, NULL)
> +const char *resolve_gitdir_gently(const char *suspect, int *return_error_code, struct strbuf *buf);
>
> void setup_work_tree(void);
>
> diff --git a/submodule.c b/submodule.c
> index 213da79f66..455444321b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
> int ret = 0;
> char *gitdir = xstrfmt("%s/.git", path);
>
> - if (resolve_gitdir_gently(gitdir, return_error_code))
> + struct strbuf resolved_gitdir_buf = STRBUF_INIT;
> + if (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))
> ret = 1;
> -
> + strbuf_release(&resolved_gitdir_buf);
> free(gitdir);
> return ret;
> }
> @@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
> {
> struct child_process cp = CHILD_PROCESS_INIT;
> struct strbuf buf = STRBUF_INIT;
> + struct strbuf gitdirbuf = STRBUF_INIT;
> FILE *fp;
> unsigned dirty_submodule = 0;
> const char *git_dir;
> int ignore_cp_exit_code = 0;
>
> strbuf_addf(&buf, "%s/.git", path);
> - git_dir = read_gitfile(buf.buf);
> + git_dir = read_gitfile_gently(buf.buf, NULL, &gitdirbuf);
> if (!git_dir)
> git_dir = buf.buf;
> if (!is_git_directory(git_dir)) {
> if (is_directory(git_dir))
> die(_("'%s' not recognized as a git repository"), git_dir);
> strbuf_release(&buf);
> + strbuf_release(&gitdirbuf);
> /* The submodule is not checked out, so it is not modified */
> return 0;
> }
> + strbuf_release(&gitdirbuf);
> strbuf_reset(&buf);
>
> strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
> @@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)
> {
> struct child_process cp = CHILD_PROCESS_INIT;
> struct strbuf buf = STRBUF_INIT;
> - const char *git_dir;
> + struct strbuf gitfilebuf = STRBUF_INIT;
>
> strbuf_addf(&buf, "%s/.git", path);
> - git_dir = read_gitfile(buf.buf);
> - if (!git_dir) {
> + read_gitfile_gently(buf.buf, NULL, &gitfilebuf);
> + if (!gitfilebuf.buf) {
> strbuf_release(&buf);
> return 0;
> }
> strbuf_release(&buf);
> + strbuf_release(&gitfilebuf);
>
> /* Now test that all nested submodules use a gitfile too */
> strvec_pushl(&cp.args,
> @@ -2276,6 +2281,7 @@ static void relocate_single_git_dir_into_superproject(const char *path,
> {
> char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
> struct strbuf new_gitdir = STRBUF_INIT;
> + struct strbuf gitfilebuf = STRBUF_INIT;
> const struct submodule *sub;
>
> if (submodule_uses_worktrees(path))
> @@ -2283,9 +2289,12 @@ static void relocate_single_git_dir_into_superproject(const char *path,
> "more than one worktree not supported"), path);
>
> old_git_dir = xstrfmt("%s/.git", path);
> - if (read_gitfile(old_git_dir))
> + if (read_gitfile_gently(old_git_dir, NULL, &gitfilebuf)) {
> /* If it is an actual gitfile, it doesn't need migration. */
> + strbuf_release(&gitfilebuf);
> return;
> + }
> + strbuf_release(&gitfilebuf);
>
> real_old_git_dir = real_pathdup(old_git_dir, 1);
>
> @@ -2343,8 +2352,9 @@ void absorb_git_dir_into_superproject(const char *path,
> int err_code;
> const char *sub_git_dir;
> struct strbuf gitdir = STRBUF_INIT;
> + struct strbuf resolved_gitdir_buf = STRBUF_INIT;
> strbuf_addf(&gitdir, "%s/.git", path);
> - sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);
> + sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code, &resolved_gitdir_buf);
>
> /* Not populated? */
> if (!sub_git_dir) {
> @@ -2385,6 +2395,8 @@ void absorb_git_dir_into_superproject(const char *path,
> free(real_sub_git_dir);
> free(real_common_git_dir);
> }
> +
> + strbuf_release(&resolved_gitdir_buf);
> strbuf_release(&gitdir);
>
> absorb_git_dir_into_superproject_recurse(path, super_prefix);
> @@ -2484,17 +2496,19 @@ int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
> const struct submodule *sub;
> const char *git_dir;
> int ret = 0;
> + struct strbuf gitfilebuf = STRBUF_INIT;
>
> strbuf_reset(buf);
> strbuf_addstr(buf, submodule);
> strbuf_complete(buf, '/');
> strbuf_addstr(buf, ".git");
>
> - git_dir = read_gitfile(buf->buf);
> + git_dir = read_gitfile_gently(buf->buf, NULL, &gitfilebuf);
> if (git_dir) {
> strbuf_reset(buf);
> strbuf_addstr(buf, git_dir);
> }
> + strbuf_release(&gitfilebuf);
> if (!is_git_directory(buf->buf)) {
> sub = submodule_from_path(the_repository, null_oid(),
> submodule);
> diff --git a/worktree.c b/worktree.c
> index b02a05a74a..a6f125c8da 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -309,7 +309,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
> {
> struct strbuf wt_path = STRBUF_INIT;
> struct strbuf realpath = STRBUF_INIT;
> - char *path = NULL;
> + struct strbuf gitfile = STRBUF_INIT;
> int err, ret = -1;
>
> strbuf_addf(&wt_path, "%s/.git", wt->path);
> @@ -353,21 +353,20 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
> goto done;
> }
>
> - path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
> - if (!path) {
> + if (!read_gitfile_gently(wt_path.buf, &err, &gitfile)) {
> strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error code %d"),
> wt_path.buf, err);
> goto done;
> }
>
> strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);
> - ret = fspathcmp(path, realpath.buf);
> + ret = fspathcmp(gitfile.buf, realpath.buf);
>
> if (ret)
> strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
> wt->path, git_common_path("worktrees/%s", wt->id));
> done:
> - free(path);
> + strbuf_release(&gitfile);
> strbuf_release(&wt_path);
> strbuf_release(&realpath);
> return ret;
> @@ -567,7 +566,7 @@ static void repair_gitfile(struct worktree *wt,
> {
> struct strbuf dotgit = STRBUF_INIT;
> struct strbuf repo = STRBUF_INIT;
> - char *backlink;
> + struct strbuf backlink = STRBUF_INIT;
> const char *repair = NULL;
> int err;
>
> @@ -582,13 +581,13 @@ static void repair_gitfile(struct worktree *wt,
>
> strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
> strbuf_addf(&dotgit, "%s/.git", wt->path);
> - backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> + read_gitfile_gently(dotgit.buf, &err, &backlink);
>
> if (err == READ_GITFILE_ERR_NOT_A_FILE)
> fn(1, wt->path, _(".git is not a file"), cb_data);
> else if (err)
> repair = _(".git file broken");
> - else if (fspathcmp(backlink, repo.buf))
> + else if (fspathcmp(backlink.buf, repo.buf))
> repair = _(".git file incorrect");
>
> if (repair) {
> @@ -596,7 +595,7 @@ static void repair_gitfile(struct worktree *wt,
> write_file(dotgit.buf, "gitdir: %s", repo.buf);
> }
>
> - free(backlink);
> + strbuf_release(&backlink);
> strbuf_release(&repo);
> strbuf_release(&dotgit);
> }
> @@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,
> struct strbuf realdotgit = STRBUF_INIT;
> struct strbuf gitdir = STRBUF_INIT;
> struct strbuf olddotgit = STRBUF_INIT;
> - char *backlink = NULL;
> + struct strbuf backlink = STRBUF_INIT;
> const char *repair = NULL;
> int err;
>
> @@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,
> goto done;
> }
>
> - backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> + read_gitfile_gently(realdotgit.buf, &err, &backlink);
> if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> goto done;
> } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> - if (!(backlink = infer_backlink(realdotgit.buf))) {
> + if (!(backlink.buf = infer_backlink(realdotgit.buf))) {
> fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
> goto done;
> }
> @@ -715,7 +714,7 @@ void repair_worktree_at_path(const char *path,
> goto done;
> }
>
> - strbuf_addf(&gitdir, "%s/gitdir", backlink);
> + strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
> if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
> repair = _("gitdir unreadable");
> else {
> @@ -729,7 +728,7 @@ void repair_worktree_at_path(const char *path,
> write_file(gitdir.buf, "%s", realdotgit.buf);
> }
> done:
> - free(backlink);
> + strbuf_release(&backlink);
> strbuf_release(&olddotgit);
> strbuf_release(&gitdir);
> strbuf_release(&realdotgit);
^ permalink raw reply
* Re: [RFC PATCH 1/3] Make read_gitfile and resolve_gitfile thread safe
From: Eric Sunshine @ 2024-03-05 4:29 UTC (permalink / raw)
To: Atneya Nair; +Cc: git, gitster, jeffhost, me, nasamuffin
In-Reply-To: <20240305012112.1598053-3-atneya@google.com>
On Mon, Mar 4, 2024 at 8:22 PM Atneya Nair <atneya@google.com> wrote:
> Continue the work in commit: 3d7747e318532a36a263c61cdf92f2decb6424ff
> to remove unsafe shared buffer usage in read_gitfile_gently.
>
> Migrate read_gitfile_gently and resolve_gitfile_gently to take strbuf
> out params to allocate their return values into, rather than returning
> a view into a shared buffer.
>
> Leave the shared buffer in case a caller passes null for this param (for
> cases we haven't cleaned up yet).
>
> Migrate callers of resolve_gitfile to resolve_gitfile_gently.
>
> Signed-off-by: Atneya Nair <atneya@google.com>
> ---
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> @@ -198,11 +198,11 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> - const char *p;
> + struct strbuf gitfile = STRBUF_INIT;
>
> - p = read_gitfile_gently(git_dir, &err);
> - if (p && get_common_dir(&sb, p)) {
> + read_gitfile_gently(git_dir, &err, &gitfile);
> + if (!err && get_common_dir(&sb, gitfile.buf)) {
> struct strbuf mainwt = STRBUF_INIT;
If you're going to adopt this idiom of checking `err` rather than the
return code of read_gitfile_gently(), then you should document that
`err` will be set to zero in the success case. Presently, the
documentation for read_gitfile_gently() only talks about the failure
case and doesn't mention that zero indicates success.
> diff --git a/setup.c b/setup.c
> @@ -830,7 +833,8 @@ void read_gitfile_error_die(int error_code, const char *path, const char *dir)
> /*
> * Try to read the location of the git directory from the .git file,
> - * return path to git directory if found. The return value comes from
> + * return path to git directory if found. If passed a valid strbuf, the return
> + * value is is a ptr to within the buffer. If strbuf is null, the return value comes from
> * a shared buffer.
What is "a valid strbuf"? Perhaps say instead "if `result_buf` is not
NULL, ...". The "is not NULL" wording is consistent with the existing
wording used below...
Also...
s/is is/is/
s/ptr/pointer/
> * On failure, if return_error_code is not NULL, return_error_code
... "is not NULL" wording is already used here.
> @@ -848,7 +852,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> - static struct strbuf realpath = STRBUF_INIT;
> + static struct strbuf shared = STRBUF_INIT;
> + if (!result_buf) {
> + result_buf = &shared;
> + }
Junio mentioned style violations in his response. Omit braces around
one line `if` bodies.
> @@ -900,8 +907,10 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
> - strbuf_realpath(&realpath, dir, 1);
> - path = realpath.buf;
> + strbuf_realpath(result_buf, dir, 1);
> + path = result_buf->buf;
It's a minor thing, but if you name the function argument `realpath`,
then the diff becomes less noisy since changes such as these do not
need to be made. On the other hand, if `realpath` isn't a good output
variable name, then by all means choose a better name.
> @@ -1316,12 +1325,13 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> + struct strbuf gitdirenvbuf = STRBUF_INIT;
> gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
> - NULL : &error_code);
> + NULL : &error_code, &gitdirenvbuf);
> if (!gitdirenv) {
> @@ -1330,8 +1340,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> - } else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
> + } else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
> + strbuf_release(&gitdirenvbuf);
> return GIT_DIR_INVALID_GITFILE;
> + }
Releasing the strbuf before `return`. Good.
> @@ -1365,9 +1377,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
> free(gitdir_path);
> free(gitfile);
> + strbuf_release(&gitdirenvbuf);
> return ret;
Likewise. Good.
> }
> + strbuf_release(&gitdirenvbuf);
>
> if (is_git_directory(dir->buf)) {
> trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
There are additional `return` statements (not shown in the context)
following this code, but you make this final strbuf_release() call
before any of those other `return` statements can be taken. Good.
> diff --git a/submodule.c b/submodule.c
> @@ -316,9 +316,10 @@ int is_submodule_populated_gently(const char *path, int *return_error_code)
> int ret = 0;
> char *gitdir = xstrfmt("%s/.git", path);
>
> - if (resolve_gitdir_gently(gitdir, return_error_code))
> + struct strbuf resolved_gitdir_buf = STRBUF_INIT;
> + if (resolve_gitdir_gently(gitdir, return_error_code, &resolved_gitdir_buf))
> ret = 1;
Style: Declare `resolved_gitdir_buf` along with `ret` and `gitdir`,
then have a blank line before the actual code.
> @@ -1879,22 +1880,25 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
> + struct strbuf gitdirbuf = STRBUF_INIT;
> + strbuf_release(&gitdirbuf);
> /* The submodule is not checked out, so it is not modified */
> return 0;
> }
> + strbuf_release(&gitdirbuf);
> strbuf_reset(&buf);
Style: Strange indentation?
> @@ -1958,15 +1962,16 @@ int submodule_uses_gitfile(const char *path)
> - const char *git_dir;
> + struct strbuf gitfilebuf = STRBUF_INIT;
>
> strbuf_addf(&buf, "%s/.git", path);
> - git_dir = read_gitfile(buf.buf);
> - if (!git_dir) {
> + read_gitfile_gently(buf.buf, NULL, &gitfilebuf);
> + if (!gitfilebuf.buf) {
> strbuf_release(&buf);
> return 0;
> }
Not sure what you're trying to do here. strbuf guarantees that its
`buf` member will never be NULL, so the new `if (!gitfilebuf.buf)`
conditional seems to be dead code. If you really want to check whether
an error occurred, pass non-NULL for the second argument and check the
return value of read_gitfile_gently() or check the error code.
> diff --git a/worktree.c b/worktree.c
> @@ -685,7 +684,7 @@ void repair_worktree_at_path(const char *path,
> - char *backlink = NULL;
> + struct strbuf backlink = STRBUF_INIT;
> @@ -701,12 +700,12 @@ void repair_worktree_at_path(const char *path,
> - backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> + read_gitfile_gently(realdotgit.buf, &err, &backlink);
> if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> goto done;
> } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> - if (!(backlink = infer_backlink(realdotgit.buf))) {
> + if (!(backlink.buf = infer_backlink(realdotgit.buf))) {
Don't do this. Never modify the internal state of strbuf directly;
consider the state read-only. Modifications should only be made via
the API. You'll need to rewrite this code a bit to make it work
correctly with the changes proposed by this patch.
^ permalink raw reply
* Re: [PATCH] gpg-interface: Look up signing keys with only email address
From: Junio C Hamano @ 2024-03-05 5:20 UTC (permalink / raw)
To: Marco Sirabella; +Cc: git
In-Reply-To: <20240305020815.2980-1-marco@sirabella.org>
Marco Sirabella <marco@sirabella.org> writes:
> Sometimes gpg signing key UIDs include a comment between the name and
> parenthesis in the form of:
>
> John Smith (example) jsmith@example.com
>
> There's no way for git to find signing keys associated with just these
> UIDs, so look up a partial match on the email only.
This codepath is about finding the key for the current user who is
in control of what committer is found by the git_committer_info()
call, no? It is not like we are finding the key that corresponds to
a random name and e-mail address on "From:" line, right?
Assuming that it is the case, it is unclear where the claim "There's
no way for git to find" comes from. It isn't a statement that is so
obviously true, at least to me, without some explanation.
> - return git_committer_info(IDENT_STRICT | IDENT_NO_DATE);
> + return git_committer_info(IDENT_NO_NAME | IDENT_STRICT | IDENT_NO_DATE);
With this change, those who use more than one identities associated
with the same e-mail address, but different human-readable names,
will get their workflow broken, won't they?
They may be using "Will <a@dd.re.ss>" when they work on a project,
while using "Bill <a@dd.re.ss>" for another project, configured via
their .git/config in the repositories used for these two projects.
And each name+address combination they have may map to a distinct
GPG key, used to sign for each project. With this change, since the
call no longer returns the name they were using to differentiate the
two keys, one of the projects they manage would lose out, no?
In general, there may be more than one signing keys that correspond
to a given human user, and the user may choose a specific key based
on what project the signatures are made for, and in such a case,
with or without the proposed change, the fallback code based on the
git_committer_info() is an unreliable way to specify the key to be
used, especially for an action as important as cryptographic
signing. I have always assumed that people use something like this
in practice:
$ git config user.signingkey = 955980DA!
that can be used to more reliably identify a specific key in their
keyring, among the ones associated with similar identifiers meant
for human consumption such as name/e-mail pairs.
So, I am not convinced that the patch is trying to address the right
problem, and I have a mild suspicion that the proposed solution to
tweak NO_NAME may simply be robbing Peter to pay Paul. If you have
named your GPG key in your keyring with a name-email pair that is
different from how you configured your committer identity, the right
solution for such a case already exists in the form of user.signingkey
configuration variable already.
Thanks.
^ permalink raw reply
* Re: Should --update-refs exclude refs pointing to the current HEAD?
From: Stefan Haller @ 2024-03-05 7:40 UTC (permalink / raw)
To: git; +Cc: Derrick Stolee, Elijah Newren, Phillip Wood, Christian Couder
In-Reply-To: <adb7f680-5bfa-6fa5-6d8a-61323fee7f53@haller-berlin.de>
On 17.04.23 10:21, Stefan Haller wrote:
> The --update-refs option of git rebase is so useful that I have it on by
> default in my config. For stacked branches I find it hard to think of
> scenarios where I wouldn't want it.
>
> However, there are cases for non-stacked branches (i.e. other branches
> pointing at the current HEAD) where updating them is undesirable. In
> fact, pretty much always, for me. Two examples, both very similar:
>
> 1. I have a topic branch which is based off of master; I want to make a
> copy of that branch and rebase it onto devel, just to try if that would
> work. I don't want the original branch to be moved along in this case.
>
> 2. I have a topic branch, and I want to make a copy of it to make some
> heavy history rewriting experiments. Again, my interactive rebases would
> always rebase both branches in the same way, not what I want. In this
> case I could work around it by doing the experiments on the original
> branch, creating a tag beforehand that I could reset back to if the
> experiments fail. But maybe I do want to keep both branches around for a
> while for some reason.
>
> Both of these cases could be fixed by --update-refs not touching any
> refs that point to the current HEAD. I'm having a hard time coming up
> with cases where you would ever want those to be updated, in fact.
Coming back to this after almost a year, I can say that I'm still
running into this problem relatively frequently, and it is annoying
every single time. Excluding refs pointing at the current head from
being updated, as proposed above, would be a big usability improvement
for me.
And I now see that "git replay --contained --onto" has the same problem,
which I find very unfortunate. In my opinion, "contained" should only
include refs that form a stack, but not copies of the current branch.
Of course, since branch stacks are only a heuristic and not a built-in
concept, it's impossible for git to distinguish between a pair of copied
branches and a degenerate stack whose top-most branch is (still) empty,
as in the example in [1]. In my personal experience though, degenerate
stacks like that are very rare, but copied branches are not, so for me
it would make a lot of sense to change the behavior of both "rebase
--update-refs" and "replay --contained".
-Stefan
[1] <https://public-inbox.org/git/
98548a5b-7d30-543b-b943-fd48d8926a33@gmail.com/>
^ permalink raw reply
* Re: [PATCH] gpg-interface: Look up signing keys with only email address
From: Marco Sirabella @ 2024-03-05 7:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqy1ax5k5w.fsf@gitster.g>
> > Sometimes gpg signing key UIDs include a comment between the name and
> > parenthesis in the form of:
> >
> > John Smith (example) jsmith@example.com
> >
> > There's no way for git to find signing keys associated with just these
> > UIDs, so look up a partial match on the email only.
>
> This codepath is about finding the key for the current user who is
> in control of what committer is found by the git_committer_info()
> call, no? It is not like we are finding the key that corresponds to
> a random name and e-mail address on "From:" line, right?
>
> Assuming that it is the case, it is unclear where the claim "There's
> no way for git to find" comes from. It isn't a statement that is so
> obviously true, at least to me, without some explanation.
Hi, yeah. This statement was in referral to specifically gpg UIDs with
comments, the parenthetical that finds itself between the full name &
email.
A workaround could be to set the committer name to "Full Name (comment)"
but that would end up being embedded in the git committer info too.
> > - return git_committer_info(IDENT_STRICT | IDENT_NO_DATE);
> > + return git_committer_info(IDENT_NO_NAME | IDENT_STRICT | IDENT_NO_DATE);
>
> With this change, those who use more than one identities associated
> with the same e-mail address, but different human-readable names,
> will get their workflow broken, won't they?
>
> They may be using "Will <a@dd.re.ss>" when they work on a project,
> while using "Bill <a@dd.re.ss>" for another project, configured via
> their .git/config in the repositories used for these two projects.
> And each name+address combination they have may map to a distinct
> GPG key, used to sign for each project. With this change, since the
> call no longer returns the name they were using to differentiate the
> two keys, one of the projects they manage would lose out, no?
This is true, yes. This is a backwards incompatible change and if you
think there's a reasonable / used use case for multiple GPG keys with
the same emails but different names then yeah, this commit breaks that
workflow.
But also as you've mentioned user.signingkey is a very easy workaround
to resolve this.
> So, I am not convinced that the patch is trying to address the right
> problem, and I have a mild suspicion that the proposed solution to
> tweak NO_NAME may simply be robbing Peter to pay Paul. If you have
> named your GPG key in your keyring with a name-email pair that is
> different from how you configured your committer identity, the right
> solution for such a case already exists in the form of user.signingkey
> configuration variable already.
I currently have a GPG key in my keyring with an identical name-email
pair to my git committer info, but it has an *additional* gpg UID
comment that introdues text within parenthesis that falls between the
owner full name & email and results in git's key lookup not working.
I appreciate the swift response!
--
Marco Sirabella
^ permalink raw reply
* Clarify the meaning of "character" in the documentation
From: Manlio Perillo @ 2024-03-05 8:43 UTC (permalink / raw)
To: git
The term "character" is confusing: does it mean 7bit/ASCII character
or Unicode Code Point?
As an example, with
git config --add core.commentChar • // Bullet (U+2022)
git does not complain, but it is rejected later.
A counter example is using UTF-8 with "user.name", where it is handled
correctly.
I sent this email after reading the documentation of "git diff
--color-moved=blocks, where the text says:
> Blocks of moved text of at least 20 alphanumeric characters are detected greedily.
In this case it is not clear if the number of characters are counted
as UTF-8 or normal 8bit bytes.
Thanks
Manlio Perillo
^ 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