Merge branch 'ps/ci-fuzzers-at-gitlab-fix'
CI fix.
* ps/ci-fuzzers-at-gitlab-fix:
gitlab-ci: fix installing dependencies for fuzz smoke tests
gitlab-ci: add smoke test for fuzzers
CI fix.
* ps/ci-fuzzers-at-gitlab-fix:
gitlab-ci: fix installing dependencies for fuzz smoke tests
gitlab-ci: add smoke test for fuzzers
Merge branch 'jk/ci-test-with-jgit-fix'
CI fix.
* jk/ci-test-with-jgit-fix:
ci: update coverity runs_on_pool reference
CI fix.
* jk/ci-test-with-jgit-fix:
ci: update coverity runs_on_pool reference
Merge branch 'jk/ci-macos-gcc13-fix'
CI fix.
* jk/ci-macos-gcc13-fix:
ci: stop installing "gcc-13" for osx-gcc
ci: avoid bare "gcc" for osx-gcc job
ci: drop mention of BREW_INSTALL_PACKAGES variable
CI fix.
* jk/ci-macos-gcc13-fix:
ci: stop installing "gcc-13" for osx-gcc
ci: avoid bare "gcc" for osx-gcc job
ci: drop mention of BREW_INSTALL_PACKAGES variable
Merge branch 'jc/no-default-attr-tree-in-bare'
Git 2.43 started using the tree of HEAD as the source of attributes
in a bare repository, which has severe performance implications.
For now, revert the change, without ripping out a more explicit
support for the attr.tree configuration variable.
* jc/no-default-attr-tree-in-bare:
stop using HEAD for attributes in bare repository by default
Git 2.43 started using the tree of HEAD as the source of attributes
in a bare repository, which has severe performance implications.
For now, revert the change, without ripping out a more explicit
support for the attr.tree configuration variable.
* jc/no-default-attr-tree-in-bare:
stop using HEAD for attributes in bare repository by default
Merge branch 'ps/ci-python-2-deprecation'
Unbreak CI jobs so that we do not attempt to use Python 2 that has
been removed from the platform.
* ps/ci-python-2-deprecation:
ci: fix Python dependency on Ubuntu 24.04
Unbreak CI jobs so that we do not attempt to use Python 2 that has
been removed from the platform.
* ps/ci-python-2-deprecation:
ci: fix Python dependency on Ubuntu 24.04
Merge branch 'tb/attr-limits'
The maximum size of attribute files is enforced more consistently.
* tb/attr-limits:
attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()
The maximum size of attribute files is enforced more consistently.
* tb/attr-limits:
attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()
Merge branch 'jc/test-workaround-broken-mv'
Tests that try to corrupt in-repository files in chunked format did
not work well on macOS due to its broken "mv", which has been
worked around.
* jc/test-workaround-broken-mv:
t/lib-chunk: work around broken "mv" on some vintage of macOS
Tests that try to corrupt in-repository files in chunked format did
not work well on macOS due to its broken "mv", which has been
worked around.
* jc/test-workaround-broken-mv:
t/lib-chunk: work around broken "mv" on some vintage of macOS
Merge branch 'ma/win32-unix-domain-socket'
Build fix.
* ma/win32-unix-domain-socket:
win32: fix building with NO_UNIX_SOCKETS
Build fix.
* ma/win32-unix-domain-socket:
win32: fix building with NO_UNIX_SOCKETS
compat/regex: fix argument order to calloc(3)
Windows compiler suddenly started complaining that calloc(3) takes
its arguments in <nmemb, size> order. Indeed, there are many calls
that has their arguments in a _wrong_ order.
Fix them all.
A sample breakage can be seen at
https://github.com/git/git/actions/runs/9046793153/job/24857988702#step:4:272
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Windows compiler suddenly started complaining that calloc(3) takes
its arguments in <nmemb, size> order. Indeed, there are many calls
that has their arguments in a _wrong_ order.
Fix them all.
A sample breakage can be seen at
https://github.com/git/git/actions/runs/9046793153/job/24857988702#step:4:272
Signed-off-by: Junio C Hamano <gitster@pobox.com>
SubmittingPatches: welcome the new maintainer of git-gui part
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Merge branch 'ps/config-subcommands' into ps/builtin-config-cleanup
* ps/config-subcommands:
builtin/config: display subcommand help
builtin/config: introduce "edit" subcommand
builtin/config: introduce "remove-section" subcommand
builtin/config: introduce "rename-section" subcommand
builtin/config: introduce "unset" subcommand
builtin/config: introduce "set" subcommand
builtin/config: introduce "get" subcommand
builtin/config: introduce "list" subcommand
builtin/config: pull out function to handle `--null`
builtin/config: pull out function to handle config location
builtin/config: use `OPT_CMDMODE()` to specify modes
builtin/config: move "fixed-value" option to correct group
builtin/config: move option array around
config: clarify memory ownership when preparing comment strings
* ps/config-subcommands:
builtin/config: display subcommand help
builtin/config: introduce "edit" subcommand
builtin/config: introduce "remove-section" subcommand
builtin/config: introduce "rename-section" subcommand
builtin/config: introduce "unset" subcommand
builtin/config: introduce "set" subcommand
builtin/config: introduce "get" subcommand
builtin/config: introduce "list" subcommand
builtin/config: pull out function to handle `--null`
builtin/config: pull out function to handle config location
builtin/config: use `OPT_CMDMODE()` to specify modes
builtin/config: move "fixed-value" option to correct group
builtin/config: move option array around
config: clarify memory ownership when preparing comment strings
SubmittingPatches: extend the "flow" section
Explain a full lifecycle of a patch series upfront, so that it is
clear when key decisions to "accept" a series is made and how a new
patch series becomes a part of a new release.
Fold the "you need to monitor the progress of your topic" section
into the primary "patch lifecycle" section, as that is one of the
things the patch submitter is responsible for. It is not like "I
sent a patch and responded to review messages, and now it is their
problem". They need to see their patch through the patch life
cycle.
Earlier versions of this document outlined a slightly different
patch flow in an idealized world, where the original submitter
gathered agreements from the participants of the discussion and sent
the final "we all agreed that this is the good version--please
apply" patches to the maintainer. In practice, this almost never
happened. Instead, describe what flow was used in practice for the
past decade that worked well for us.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Explain a full lifecycle of a patch series upfront, so that it is
clear when key decisions to "accept" a series is made and how a new
patch series becomes a part of a new release.
Fold the "you need to monitor the progress of your topic" section
into the primary "patch lifecycle" section, as that is one of the
things the patch submitter is responsible for. It is not like "I
sent a patch and responded to review messages, and now it is their
problem". They need to see their patch through the patch life
cycle.
Earlier versions of this document outlined a slightly different
patch flow in an idealized world, where the original submitter
gathered agreements from the participants of the discussion and sent
the final "we all agreed that this is the good version--please
apply" patches to the maintainer. In practice, this almost never
happened. Instead, describe what flow was used in practice for the
past decade that worked well for us.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
SubmittingPatches: move the patch-flow section earlier
Before discussing the small details of how the patch gets sent, we'd
want to give people a larger picture first to set the expectation
straight. The existing patch-flow section covers materials that are
suitable for that purpose, so move it to the beginning of the
document. We'll update the contents of the section to clarify what
goal the patch submitter is working towards in the next step, which
will make it easier to understand the reason behind the individual
rules presented in latter parts of the document.
This step only moves two sections (patch-flow and patch-status)
without changing their contents, except that their section levels
are demoted from Level 1 to Level 2 to fit better in the document
structure at their new place.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Before discussing the small details of how the patch gets sent, we'd
want to give people a larger picture first to set the expectation
straight. The existing patch-flow section covers materials that are
suitable for that purpose, so move it to the beginning of the
document. We'll update the contents of the section to clarify what
goal the patch submitter is working towards in the next step, which
will make it easier to understand the reason behind the individual
rules presented in latter parts of the document.
This step only moves two sections (patch-flow and patch-status)
without changing their contents, except that their section levels
are demoted from Level 1 to Level 2 to fit better in the document
structure at their new place.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: stop installing "gcc-13" for osx-gcc
Our osx-gcc job explicitly asks to install gcc-13. But since the GitHub
runner image already comes with gcc-13 installed, this is mostly doing
nothing (or in some cases it may install an incremental update over the
runner image). But worse, it recently started causing errors like:
==> Fetching gcc@13
==> Downloading https://ghcr.io/v2/homebrew/core/gcc/13/blobs/sha256:fb2403d97e2ce67eb441b54557cfb61980830f3ba26d4c5a1fe5ecd0c9730d1a
==> Pouring gcc@13--13.2.0.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/c++-13
Target /usr/local/bin/c++-13
is a symlink belonging to gcc. You can unlink it:
brew unlink gcc
which cause the whole CI job to bail.
I didn't track down the root cause, but I suspect it may be related to
homebrew recently switching the "gcc" default to gcc-14. And it may even
be fixed when a new runner image is released. But if we don't need to
run brew at all, it's one less thing for us to worry about.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Our osx-gcc job explicitly asks to install gcc-13. But since the GitHub
runner image already comes with gcc-13 installed, this is mostly doing
nothing (or in some cases it may install an incremental update over the
runner image). But worse, it recently started causing errors like:
==> Fetching gcc@13
==> Downloading https://ghcr.io/v2/homebrew/core/gcc/13/blobs/sha256:fb2403d97e2ce67eb441b54557cfb61980830f3ba26d4c5a1fe5ecd0c9730d1a
==> Pouring gcc@13--13.2.0.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/c++-13
Target /usr/local/bin/c++-13
is a symlink belonging to gcc. You can unlink it:
brew unlink gcc
which cause the whole CI job to bail.
I didn't track down the root cause, but I suspect it may be related to
homebrew recently switching the "gcc" default to gcc-14. And it may even
be fixed when a new runner image is released. But if we don't need to
run brew at all, it's one less thing for us to worry about.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: avoid bare "gcc" for osx-gcc job
On macOS, a bare "gcc" (without a version) will invoke a wrapper for
clang, not actual gcc. Even when gcc is installed via homebrew, that
only provides version-specific links in /usr/local/bin (like "gcc-13"),
and never a version-agnostic "gcc" wrapper.
As far as I can tell, this has been the case for a long time, and this
osx-gcc job has largely been doing nothing. We can point it at "gcc-13",
which will pick up the homebrew-installed version.
The fix here is specific to the github workflow file, as the gitlab one
does not have a matching job.
It's a little unfortunate that we cannot just ask for the latest version
of gcc which homebrew provides, but as far as I can tell there is no
easy alias (you'd have to find the highest number gcc-* in
/usr/local/bin yourself).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
On macOS, a bare "gcc" (without a version) will invoke a wrapper for
clang, not actual gcc. Even when gcc is installed via homebrew, that
only provides version-specific links in /usr/local/bin (like "gcc-13"),
and never a version-agnostic "gcc" wrapper.
As far as I can tell, this has been the case for a long time, and this
osx-gcc job has largely been doing nothing. We can point it at "gcc-13",
which will pick up the homebrew-installed version.
The fix here is specific to the github workflow file, as the gitlab one
does not have a matching job.
It's a little unfortunate that we cannot just ask for the latest version
of gcc which homebrew provides, but as far as I can tell there is no
easy alias (you'd have to find the highest number gcc-* in
/usr/local/bin yourself).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: drop mention of BREW_INSTALL_PACKAGES variable
The last user of this variable went away in 4a6e4b9602 (CI: remove
Travis CI support, 2021-11-23), so it's doing nothing except making it
more confusing to find out which packages _are_ installed.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The last user of this variable went away in 4a6e4b9602 (CI: remove
Travis CI support, 2021-11-23), so it's doing nothing except making it
more confusing to find out which packages _are_ installed.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: update coverity runs_on_pool reference
Commit 2d65e5b6a6 (ci: rename "runs_on_pool" to "distro", 2024-04-12)
renamed this variable for the main CI workflow, as well as in the ci/
scripts. Because the coverity workflow also relies on those scripts to
install dependencies, it needs to be updated, too. Without this patch,
the coverity build fails because we lack libcurl.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commit 2d65e5b6a6 (ci: rename "runs_on_pool" to "distro", 2024-04-12)
renamed this variable for the main CI workflow, as well as in the ci/
scripts. Because the coverity workflow also relies on those scripts to
install dependencies, it needs to be updated, too. Without this patch,
the coverity build fails because we lack libcurl.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitlab-ci: fix installing dependencies for fuzz smoke tests
There was a semantic merge conflict between 9cdeb34b96 (ci: merge
scripts which install dependencies, 2024-04-12), which has merged
"ci/install-docker-dependencies.sh" into "ci/install-dependencies.sh"
and c7b228e000 (gitlab-ci: add smoke test for fuzzers, 2024-04-29),
which has added a new fuzz smoke test job that makes use of the
now-removed script.
Adapt the job to instead use the new script to install dependencies.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There was a semantic merge conflict between 9cdeb34b96 (ci: merge
scripts which install dependencies, 2024-04-12), which has merged
"ci/install-docker-dependencies.sh" into "ci/install-dependencies.sh"
and c7b228e000 (gitlab-ci: add smoke test for fuzzers, 2024-04-29),
which has added a new fuzz smoke test job that makes use of the
now-removed script.
Adapt the job to instead use the new script to install dependencies.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Merge branch 'ps/ci-python-2-deprecation' into ps/ci-fuzzers-at-gitlab-fix
* ps/ci-python-2-deprecation:
ci: fix Python dependency on Ubuntu 24.04
* ps/ci-python-2-deprecation:
ci: fix Python dependency on Ubuntu 24.04
Merge branch 'ps/ci-enable-minimal-fuzzers-at-gitlab' into ps/ci-fuzzers-at-gitlab-fix
* ps/ci-enable-minimal-fuzzers-at-gitlab:
gitlab-ci: add smoke test for fuzzers
* ps/ci-enable-minimal-fuzzers-at-gitlab:
gitlab-ci: add smoke test for fuzzers
git-p4: show Perforce error to the user
During "git p4 clone" if p4 process returns an error from the server,
it will store the message in the 'err' variable. Then it will send a
text command "die-now" to git-fast-import. However, git-fast-import
raises an exception: "fatal: Unsupported command: die-now" and err is
never displayed. This patch ensures that err is shown to the end user.
Signed-off-by: Fahad Alrashed <fahad@keylock.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
During "git p4 clone" if p4 process returns an error from the server,
it will store the message in the 'err' variable. Then it will send a
text command "die-now" to git-fast-import. However, git-fast-import
raises an exception: "fatal: Unsupported command: die-now" and err is
never displayed. This patch ensures that err is shown to the end user.
Signed-off-by: Fahad Alrashed <fahad@keylock.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The second batch
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Merge branch 'bb/rgb-12-bit-colors'
The color parsing code learned to handle 12-bit RGB colors, spelled
as "#RGB" (in addition to "#RRGGBB" that is already supported).
* bb/rgb-12-bit-colors:
color: add support for 12-bit RGB colors
t/t4026-color: add test coverage for invalid RGB colors
t/t4026-color: remove an extra double quote character
The color parsing code learned to handle 12-bit RGB colors, spelled
as "#RGB" (in addition to "#RRGGBB" that is already supported).
* bb/rgb-12-bit-colors:
color: add support for 12-bit RGB colors
t/t4026-color: add test coverage for invalid RGB colors
t/t4026-color: remove an extra double quote character
Merge branch 'rs/diff-parseopts-cleanup'
Code clean-up to remove code that is now a noop.
* rs/diff-parseopts-cleanup:
diff-lib: stop calling diff_setup_done() in do_diff_cache()
Code clean-up to remove code that is now a noop.
* rs/diff-parseopts-cleanup:
diff-lib: stop calling diff_setup_done() in do_diff_cache()
Merge branch 'dk/zsh-git-repo-path-fix'
Command line completion support for zsh (in contrib/) has been
updated to stop exposing internal state to end-user shell
interaction.
* dk/zsh-git-repo-path-fix:
completion: zsh: stop leaking local cache variable
Command line completion support for zsh (in contrib/) has been
updated to stop exposing internal state to end-user shell
interaction.
* dk/zsh-git-repo-path-fix:
completion: zsh: stop leaking local cache variable
Merge branch 'bc/zsh-compatibility'
zsh can pretend to be a normal shell pretty well except for some
glitches that we tickle in some of our scripts. Work them around
so that "vimdiff" and our test suite works well enough with it.
* bc/zsh-compatibility:
vimdiff: make script and tests work with zsh
t4046: avoid continue in &&-chain for zsh
zsh can pretend to be a normal shell pretty well except for some
glitches that we tickle in some of our scripts. Work them around
so that "vimdiff" and our test suite works well enough with it.
* bc/zsh-compatibility:
vimdiff: make script and tests work with zsh
t4046: avoid continue in &&-chain for zsh
Merge branch 'rj/add-p-typo-reaction'
When the user responds to a prompt given by "git add -p" with an
unsupported command, list of available commands were given, which
was too much if the user knew what they wanted to type but merely
made a typo. Now the user gets a much shorter error message.
* rj/add-p-typo-reaction:
add-patch: response to unknown command
add-patch: do not show UI messages on stderr
When the user responds to a prompt given by "git add -p" with an
unsupported command, list of available commands were given, which
was too much if the user knew what they wanted to type but merely
made a typo. Now the user gets a much shorter error message.
* rj/add-p-typo-reaction:
add-patch: response to unknown command
add-patch: do not show UI messages on stderr
Merge branch 'jt/doc-submitting-rerolled-series'
Developer doc update.
* jt/doc-submitting-rerolled-series:
doc: clarify practices for submitting updated patch versions
Developer doc update.
* jt/doc-submitting-rerolled-series:
doc: clarify practices for submitting updated patch versions
Merge branch 'rh/complete-symbolic-ref'
Command line completion script (in contrib/) learned to complete
"git symbolic-ref" a bit better (you need to enable plumbing
commands to be completed with GIT_COMPLETION_SHOW_ALL_COMMANDS).
* rh/complete-symbolic-ref:
completion: add docs on how to add subcommand completions
completion: improve docs for using __git_complete
completion: add 'symbolic-ref'
Command line completion script (in contrib/) learned to complete
"git symbolic-ref" a bit better (you need to enable plumbing
commands to be completed with GIT_COMPLETION_SHOW_ALL_COMMANDS).
* rh/complete-symbolic-ref:
completion: add docs on how to add subcommand completions
completion: improve docs for using __git_complete
completion: add 'symbolic-ref'
Merge branch 'ps/the-index-is-no-more'
The singleton index_state instance "the_index" has been eliminated
by always instantiating "the_repository" and replacing references
to "the_index" with references to its .index member.
* ps/the-index-is-no-more:
repository: drop `initialize_the_repository()`
repository: drop `the_index` variable
builtin/clone: stop using `the_index`
repository: initialize index in `repo_init()`
builtin: stop using `the_index`
t/helper: stop using `the_index`
The singleton index_state instance "the_index" has been eliminated
by always instantiating "the_repository" and replacing references
to "the_index" with references to its .index member.
* ps/the-index-is-no-more:
repository: drop `initialize_the_repository()`
repository: drop `the_index` variable
builtin/clone: stop using `the_index`
repository: initialize index in `repo_init()`
builtin: stop using `the_index`
t/helper: stop using `the_index`
Merge branch 'bc/credential-scheme-enhancement'
The credential helper protocol, together with the HTTP layer, have
been enhanced to support authentication schemes different from
username & password pair, like Bearer and NTLM.
* bc/credential-scheme-enhancement:
credential: add method for querying capabilities
credential-cache: implement authtype capability
t: add credential tests for authtype
credential: add support for multistage credential rounds
t5563: refactor for multi-stage authentication
docs: set a limit on credential line length
credential: enable state capability
credential: add an argument to keep state
http: add support for authtype and credential
docs: indicate new credential protocol fields
credential: add a field called "ephemeral"
credential: gate new fields on capability
credential: add a field for pre-encoded credentials
http: use new headers for each object request
remote-curl: reset headers on new request
credential: add an authtype field
The credential helper protocol, together with the HTTP layer, have
been enhanced to support authentication schemes different from
username & password pair, like Bearer and NTLM.
* bc/credential-scheme-enhancement:
credential: add method for querying capabilities
credential-cache: implement authtype capability
t: add credential tests for authtype
credential: add support for multistage credential rounds
t5563: refactor for multi-stage authentication
docs: set a limit on credential line length
credential: enable state capability
credential: add an argument to keep state
http: add support for authtype and credential
docs: indicate new credential protocol fields
credential: add a field called "ephemeral"
credential: gate new fields on capability
credential: add a field for pre-encoded credentials
http: use new headers for each object request
remote-curl: reset headers on new request
credential: add an authtype field
Merge branch 'ps/ci-test-with-jgit'
Tests to ensure interoperability between reftable written by jgit
and our code have been added and enabled in CI.
* ps/ci-test-with-jgit:
t0612: add tests to exercise Git/JGit reftable compatibility
t0610: fix non-portable variable assignment
t06xx: always execute backend-specific tests
ci: install JGit dependency
ci: make Perforce binaries executable for all users
ci: merge scripts which install dependencies
ci: fix setup of custom path for GitLab CI
ci: merge custom PATH directories
ci: convert "install-dependencies.sh" to use "/bin/sh"
ci: drop duplicate package installation for "linux-gcc-default"
ci: skip sudo when we are already root
ci: expose distro name in dockerized GitHub jobs
ci: rename "runs_on_pool" to "distro"
Tests to ensure interoperability between reftable written by jgit
and our code have been added and enabled in CI.
* ps/ci-test-with-jgit:
t0612: add tests to exercise Git/JGit reftable compatibility
t0610: fix non-portable variable assignment
t06xx: always execute backend-specific tests
ci: install JGit dependency
ci: make Perforce binaries executable for all users
ci: merge scripts which install dependencies
ci: fix setup of custom path for GitLab CI
ci: merge custom PATH directories
ci: convert "install-dependencies.sh" to use "/bin/sh"
ci: drop duplicate package installation for "linux-gcc-default"
ci: skip sudo when we are already root
ci: expose distro name in dockerized GitHub jobs
ci: rename "runs_on_pool" to "distro"
Merge branch 'ps/reftable-write-optim'
Code to write out reftable has seen some optimization and
simplification.
* ps/reftable-write-optim:
reftable/block: reuse compressed array
reftable/block: reuse zstream when writing log blocks
reftable/writer: reset `last_key` instead of releasing it
reftable/writer: unify releasing memory
reftable/writer: refactorings for `writer_flush_nonempty_block()`
reftable/writer: refactorings for `writer_add_record()`
refs/reftable: don't recompute committer ident
reftable: remove name checks
refs/reftable: skip duplicate name checks
refs/reftable: perform explicit D/F check when writing symrefs
refs/reftable: fix D/F conflict error message on ref copy
Code to write out reftable has seen some optimization and
simplification.
* ps/reftable-write-optim:
reftable/block: reuse compressed array
reftable/block: reuse zstream when writing log blocks
reftable/writer: reset `last_key` instead of releasing it
reftable/writer: unify releasing memory
reftable/writer: refactorings for `writer_flush_nonempty_block()`
reftable/writer: refactorings for `writer_add_record()`
refs/reftable: don't recompute committer ident
reftable: remove name checks
refs/reftable: skip duplicate name checks
refs/reftable: perform explicit D/F check when writing symrefs
refs/reftable: fix D/F conflict error message on ref copy
scalar: avoid segfault in reconfigure --all
During the latest v2.45.0 update, 'scalar reconfigure --all' started to
segfault on my machine. Breaking it down via the debugger, it was
faulting on a NULL reference to the_hash_algo, which is a macro pointing
to the_repository->hash_algo.
In my case, this is due to one of my repositories having a detached HEAD,
which requires get_oid_hex() to parse that the HEAD reference is valid.
Another way to cause a failure is to use the "includeIf.onbranch" config
key, which will lead to a BUG() statement.
My first inclination was to try to refactor cmd_reconfigure() to execute
'git for-each-repo' instead of this loop. In addition to the difficulty
of executing 'scalar reconfigure' within 'git for-each-repo', it would
be difficult to perform the clean-up logic for non-existent repos if we
relied on that child process.
Instead, I chose to move the temporary repo to be within the loop and
reinstate the_repository to its old value after we are done performing
logic on the current array item.
Add tests to t9210-scalar.sh to test 'scalar reconfigure --all' with
multiple registered repos. There are two different ways that the old
use of the_repository could trigger bugs. These issues are being solved
independently to be more careful about the_repository being
uninitialized, but the change in this patch around the use of
the_repository is still a good safety precaution.
Co-authored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
During the latest v2.45.0 update, 'scalar reconfigure --all' started to
segfault on my machine. Breaking it down via the debugger, it was
faulting on a NULL reference to the_hash_algo, which is a macro pointing
to the_repository->hash_algo.
In my case, this is due to one of my repositories having a detached HEAD,
which requires get_oid_hex() to parse that the HEAD reference is valid.
Another way to cause a failure is to use the "includeIf.onbranch" config
key, which will lead to a BUG() statement.
My first inclination was to try to refactor cmd_reconfigure() to execute
'git for-each-repo' instead of this loop. In addition to the difficulty
of executing 'scalar reconfigure' within 'git for-each-repo', it would
be difficult to perform the clean-up logic for non-existent repos if we
relied on that child process.
Instead, I chose to move the temporary repo to be within the loop and
reinstate the_repository to its old value after we are done performing
logic on the current array item.
Add tests to t9210-scalar.sh to test 'scalar reconfigure --all' with
multiple registered repos. There are two different ways that the old
use of the_repository could trigger bugs. These issues are being solved
independently to be more careful about the_repository being
uninitialized, but the change in this patch around the use of
the_repository is still a good safety precaution.
Co-authored-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
t0018: two small fixes
Even though the three tests that were recently added started their
here-doc with "<<-\EOF", it did not take advantage of that and
instead wrote the here-doc payload abut to the left edge. Use a tabs
to indent these lines.
More importantly, because these all hardcode the expected output,
which contains the current branch name, they break the CI job that
uses 'main' as the default branch name.
Use
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=trunk
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
between the test_description line and ". ./test-lib.sh" line to
force the initial branch name to 'trunk' and expect it to show in
the output.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Even though the three tests that were recently added started their
here-doc with "<<-\EOF", it did not take advantage of that and
instead wrote the here-doc payload abut to the left edge. Use a tabs
to indent these lines.
More importantly, because these all hardcode the expected output,
which contains the current branch name, they break the CI job that
uses 'main' as the default branch name.
Use
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=trunk
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
between the test_description line and ". ./test-lib.sh" line to
force the initial branch name to 'trunk' and expect it to show in
the output.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Documentation/git-merge-tree.txt: document -X
Add an entry in the 'merge-tree' builtin documentation for
-X/--strategy-option (added in 6a4c9e7b32 (merge-tree: add -X strategy
option, 2023-09-24)). The same option is documented for 'merge', 'rebase',
'revert', etc. in their respective Documentation/ files, so let's do the
same for 'merge-tree'.
Signed-off-by: Victoria Dye <vdye@github.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add an entry in the 'merge-tree' builtin documentation for
-X/--strategy-option (added in 6a4c9e7b32 (merge-tree: add -X strategy
option, 2023-09-24)). The same option is documented for 'merge', 'rebase',
'revert', etc. in their respective Documentation/ files, so let's do the
same for 'merge-tree'.
Signed-off-by: Victoria Dye <vdye@github.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: remove functions without ref store
The preceding commit has rewritten all callers of ref-related functions
to use the equivalents that accept a `struct ref_store`. Consequently,
the respective variants without the ref store are now unused. Remove
them.
There are likely patch series in-flight that use the now-removed
functions. To help the authors, the old implementations have been added
to "refs.c" in an ifdef'd section as a reference for how to migrate each
of the respective callers.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The preceding commit has rewritten all callers of ref-related functions
to use the equivalents that accept a `struct ref_store`. Consequently,
the respective variants without the ref store are now unused. Remove
them.
There are likely patch series in-flight that use the now-removed
functions. To help the authors, the old implementations have been added
to "refs.c" in an ifdef'd section as a reference for how to migrate each
of the respective callers.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
cocci: apply rules to rewrite callers of "refs" interfaces
Apply the rules that rewrite callers of "refs" interfaces to explicitly
pass `struct ref_store`. The resulting patch has been applied with the
`--whitespace=fix` option.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Apply the rules that rewrite callers of "refs" interfaces to explicitly
pass `struct ref_store`. The resulting patch has been applied with the
`--whitespace=fix` option.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
cocci: introduce rules to transform "refs" to pass ref store
Most of the functions in "refs.h" have two flavors: one that accepts a
`struct ref_store`, and one that figures it out via `the_repository`.
As part of the libification efforts we want to get rid of the latter
variant and stop relying on `the_repository` altogether.
Introduce a set of Coccinelle rules that transform callers of the "refs"
interfaces to pass a `struct ref_store`. These rules are not yet applied
by this patch so that it can be reviewed standalone more easily. This
will be done in the next patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Most of the functions in "refs.h" have two flavors: one that accepts a
`struct ref_store`, and one that figures it out via `the_repository`.
As part of the libification efforts we want to get rid of the latter
variant and stop relying on `the_repository` altogether.
Introduce a set of Coccinelle rules that transform callers of the "refs"
interfaces to pass a `struct ref_store`. These rules are not yet applied
by this patch so that it can be reviewed standalone more easily. This
will be done in the next patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: add `exclude_patterns` parameter to `for_each_fullref_in()`
The `for_each_fullref_in()` function is supposedly the ref-store-less
equivalent of `refs_for_each_fullref_in()`, but the latter has gained a
new parameter `exclude_patterns` over time. Bring these two functions
back in sync again by adding the parameter to the former function, as
well.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `for_each_fullref_in()` function is supposedly the ref-store-less
equivalent of `refs_for_each_fullref_in()`, but the latter has gained a
new parameter `exclude_patterns` over time. Bring these two functions
back in sync again by adding the parameter to the former function, as
well.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: introduce missing functions that accept a `struct ref_store`
While most of the functions in "refs.h" have a variant that accepts a
`struct ref_store`, some don't. Callers of these functions are thus
forced to implicitly rely on `the_repository` to figure out the ref
store that is to be used.
Introduce those missing functions to address this shortcoming.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
While most of the functions in "refs.h" have a variant that accepts a
`struct ref_store`, some don't. Callers of these functions are thus
forced to implicitly rely on `the_repository` to figure out the ref
store that is to be used.
Introduce those missing functions to address this shortcoming.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/tag: add --trailer option
git-tag supports interpreting trailers from an annotated tag message,
using --list --format="%(trailers)". However, the available methods to
add a trailer to a tag message (namely -F or --editor) are not as
ergonomic.
In a previous patch, we moved git-commit's implementation of its
--trailer option to the trailer.h API. Let's use that new function to
teach git-tag the same --trailer option, emulating as much of
git-commit's behavior as much as possible.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-tag supports interpreting trailers from an annotated tag message,
using --list --format="%(trailers)". However, the available methods to
add a trailer to a tag message (namely -F or --editor) are not as
ergonomic.
In a previous patch, we moved git-commit's implementation of its
--trailer option to the trailer.h API. Let's use that new function to
teach git-tag the same --trailer option, emulating as much of
git-commit's behavior as much as possible.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/commit: refactor --trailer logic
git-commit adds user trailers to the commit message by passing its
`--trailer` arguments to a child process running `git-interpret-trailers
--in-place`. This logic is broadly useful, not just for git-commit but
for other commands constructing message bodies (e.g. git-tag).
Let's move this logic from git-commit to a new function in the trailer
API, so that it can be re-used in other commands.
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-commit adds user trailers to the commit message by passing its
`--trailer` arguments to a child process running `git-interpret-trailers
--in-place`. This logic is broadly useful, not just for git-commit but
for other commands constructing message bodies (e.g. git-tag).
Let's move this logic from git-commit to a new function in the trailer
API, so that it can be re-used in other commands.
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/commit: use ARGV macro to collect trailers
Replace git-commit's callback for --trailer with the standard
OPT_PASSTHRU_ARGV macro. The callback only adds its values to a strvec
and sanity-checks that `unset` is always false; both of these are
already implemented in the parse-option API.
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Replace git-commit's callback for --trailer with the standard
OPT_PASSTHRU_ARGV macro. The callback only adds its values to a strvec
and sanity-checks that `unset` is always false; both of these are
already implemented in the parse-option API.
Signed-off-by: John Passaro <john.a.passaro@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: remove `create_symref` and associated dead code
In the previous commits, we converted `refs_create_symref()` to utilize
transactions to perform symref updates. Earlier `refs_create_symref()`
used `create_symref()` to do the same.
We can now remove `create_symref()` and any code associated with it
which is no longer used. We remove `create_symref()` code from all the
reference backends and also remove it entirely from the `ref_storage_be`
struct.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the previous commits, we converted `refs_create_symref()` to utilize
transactions to perform symref updates. Earlier `refs_create_symref()`
used `create_symref()` to do the same.
We can now remove `create_symref()` and any code associated with it
which is no longer used. We remove `create_symref()` code from all the
reference backends and also remove it entirely from the `ref_storage_be`
struct.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: rename `refs_create_symref()` to `refs_update_symref()`
The `refs_create_symref()` function is used to update/create a symref.
But it doesn't check the old target of the symref, if existing. It force
updates the symref. In this regard, the name `refs_create_symref()` is a
bit misleading. So let's rename it to `refs_update_symref()`. This is
akin to how 'git-update-ref(1)' also allows us to create apart from
update.
While we're here, rename the arguments in the function to clarify what
they actually signify and reduce confusion.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `refs_create_symref()` function is used to update/create a symref.
But it doesn't check the old target of the symref, if existing. It force
updates the symref. In this regard, the name `refs_create_symref()` is a
bit misleading. So let's rename it to `refs_update_symref()`. This is
akin to how 'git-update-ref(1)' also allows us to create apart from
update.
While we're here, rename the arguments in the function to clarify what
they actually signify and reduce confusion.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: use transaction in `refs_create_symref()`
The `refs_create_symref()` function updates a symref to a given new
target. To do this, it uses a ref-backend specific function
`create_symref()`.
In the previous commits, we introduced symref support in transactions.
This means we can now use transactions to perform symref updates and
don't have to resort to `create_symref()`. Doing this allows us to
remove and cleanup `create_symref()`, which we will do in the following
commit.
Modify the expected error message for a test in
't/t0610-reftable-basics.sh', since the error is now thrown from
'refs.c'. This is because in transactional updates, F/D conflicts are
caught before we're in the reference backend.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `refs_create_symref()` function updates a symref to a given new
target. To do this, it uses a ref-backend specific function
`create_symref()`.
In the previous commits, we introduced symref support in transactions.
This means we can now use transactions to perform symref updates and
don't have to resort to `create_symref()`. Doing this allows us to
remove and cleanup `create_symref()`, which we will do in the following
commit.
Modify the expected error message for a test in
't/t0610-reftable-basics.sh', since the error is now thrown from
'refs.c'. This is because in transactional updates, F/D conflicts are
caught before we're in the reference backend.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: add support for transactional symref updates
The reference backends currently support transactional reference
updates. While this is exposed to users via 'git-update-ref' and its
'--stdin' mode, it is also used internally within various commands.
However, we do not support transactional updates of symrefs. This commit
adds support for symrefs in both the 'files' and the 'reftable' backend.
Here, we add and use `ref_update_has_null_new_value()`, a helper
function which is used to check if there is a new_value in a reference
update. The new value could either be a symref target `new_target` or a
OID `new_oid`.
We also add another common function `ref_update_check_old_target` which
will be used to check if the update's old_target corresponds to a
reference's current target.
Now transactional updates (verify, create, delete, update) can be used
for:
- regular refs
- symbolic refs
- conversion of regular to symbolic refs and vice versa
This also allows us to expose this to users via new commands in
'git-update-ref' in the future.
Note that a dangling symref update does not record a new reflog entry,
which is unchanged before and after this commit.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The reference backends currently support transactional reference
updates. While this is exposed to users via 'git-update-ref' and its
'--stdin' mode, it is also used internally within various commands.
However, we do not support transactional updates of symrefs. This commit
adds support for symrefs in both the 'files' and the 'reftable' backend.
Here, we add and use `ref_update_has_null_new_value()`, a helper
function which is used to check if there is a new_value in a reference
update. The new value could either be a symref target `new_target` or a
OID `new_oid`.
We also add another common function `ref_update_check_old_target` which
will be used to check if the update's old_target corresponds to a
reference's current target.
Now transactional updates (verify, create, delete, update) can be used
for:
- regular refs
- symbolic refs
- conversion of regular to symbolic refs and vice versa
This also allows us to expose this to users via new commands in
'git-update-ref' in the future.
Note that a dangling symref update does not record a new reflog entry,
which is unchanged before and after this commit.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: move `original_update_refname` to 'refs.c'
The files backend and the reftable backend implement
`original_update_refname` to obtain the original refname of the update.
Move it out to 'refs.c' and only expose it internally to the refs
library. This will be used in an upcoming commit to also introduce
another common functionality for the two backends.
We also rename the function to `ref_update_original_update_refname` to
keep it consistent with the upcoming other 'ref_update_*' functions
that'll be introduced.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The files backend and the reftable backend implement
`original_update_refname` to obtain the original refname of the update.
Move it out to 'refs.c' and only expose it internally to the refs
library. This will be used in an upcoming commit to also introduce
another common functionality for the two backends.
We also rename the function to `ref_update_original_update_refname` to
keep it consistent with the upcoming other 'ref_update_*' functions
that'll be introduced.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: support symrefs in 'reference-transaction' hook
The 'reference-transaction' hook runs whenever a reference update is
made to the system. In a previous commit, we added the `old_target` and
`new_target` fields to the `reference_transaction_update()`. In
following commits we'll also add the code to handle symref's in the
reference backends.
Support symrefs also in the 'reference-transaction' hook, by modifying
the current format:
<old-oid> SP <new-oid> SP <ref-name> LF
to be be:
<old-value> SP <new-value> SP <ref-name> LF
where for regular refs the output would not change and remain the same.
But when either 'old-value' or 'new-value' is a symref, we print the ref
as 'ref:<ref-target>'.
This does break backward compatibility, but the 'reference-transaction'
hook's documentation always stated that support for symbolic references
may be added in the future.
We do not add any tests in this commit since there is no git command
which activates this flow, in an upcoming commit, we'll start using
transaction based symref updates as the default, we'll add tests there
for the hook too.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The 'reference-transaction' hook runs whenever a reference update is
made to the system. In a previous commit, we added the `old_target` and
`new_target` fields to the `reference_transaction_update()`. In
following commits we'll also add the code to handle symref's in the
reference backends.
Support symrefs also in the 'reference-transaction' hook, by modifying
the current format:
<old-oid> SP <new-oid> SP <ref-name> LF
to be be:
<old-value> SP <new-value> SP <ref-name> LF
where for regular refs the output would not change and remain the same.
But when either 'old-value' or 'new-value' is a symref, we print the ref
as 'ref:<ref-target>'.
This does break backward compatibility, but the 'reference-transaction'
hook's documentation always stated that support for symbolic references
may be added in the future.
We do not add any tests in this commit since there is no git command
which activates this flow, in an upcoming commit, we'll start using
transaction based symref updates as the default, we'll add tests there
for the hook too.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
files-backend: extract out `create_symref_lock()`
The function `create_symref_locked()` creates a symref by creating a
'<symref>.lock' file and then committing the symref lock, which creates
the final symref.
Extract the early half of `create_symref_locked()` into a new helper
function `create_symref_lock()`. Because the name of the new function is
too similar to the original, rename the original to
`create_and_commit_symref()` to avoid confusion.
The new function `create_symref_locked()` can be used to create the
symref lock in a separate step from that of committing it. This allows
to add transactional support for symrefs, where the lock would be
created in the preparation step and the lock would be committed in the
finish step.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `create_symref_locked()` creates a symref by creating a
'<symref>.lock' file and then committing the symref lock, which creates
the final symref.
Extract the early half of `create_symref_locked()` into a new helper
function `create_symref_lock()`. Because the name of the new function is
too similar to the original, rename the original to
`create_and_commit_symref()` to avoid confusion.
The new function `create_symref_locked()` can be used to create the
symref lock in a separate step from that of committing it. This allows
to add transactional support for symrefs, where the lock would be
created in the preparation step and the lock would be committed in the
finish step.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: accept symref values in `ref_transaction_update()`
The function `ref_transaction_update()` obtains ref information and
flags to create a `ref_update` and add them to the transaction at hand.
To extend symref support in transactions, we need to also accept the
old and new ref targets and process it. This commit adds the required
parameters to the function and modifies all call sites.
The two parameters added are `new_target` and `old_target`. The
`new_target` is used to denote what the reference should point to when
the transaction is applied. Some functions allow this parameter to be
NULL, meaning that the reference is not changed.
The `old_target` denotes the value the reference must have before the
update. Some functions allow this parameter to be NULL, meaning that the
old value of the reference is not checked.
We also update the internal function `ref_transaction_add_update()`
similarly to take the two new parameters.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `ref_transaction_update()` obtains ref information and
flags to create a `ref_update` and add them to the transaction at hand.
To extend symref support in transactions, we need to also accept the
old and new ref targets and process it. This commit adds the required
parameters to the function and modifies all call sites.
The two parameters added are `new_target` and `old_target`. The
`new_target` is used to denote what the reference should point to when
the transaction is applied. Some functions allow this parameter to be
NULL, meaning that the reference is not changed.
The `old_target` denotes the value the reference must have before the
update. Some functions allow this parameter to be NULL, meaning that the
old value of the reference is not checked.
We also update the internal function `ref_transaction_add_update()`
similarly to take the two new parameters.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
cmake: let `test-tool` run the unit tests, too
The `test-tool` recently learned to run the unit tests. To this end, it
needs to link with `test-lib.c`, which was done in the `Makefile`, and
this patch does it in the CMake definition, too.
This is a companion of 44400f58407e (t0080: turn t-basic unit test into
a helper, 2024-02-02).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `test-tool` recently learned to run the unit tests. To this end, it
needs to link with `test-lib.c`, which was done in the `Makefile`, and
this patch does it in the CMake definition, too.
This is a companion of 44400f58407e (t0080: turn t-basic unit test into
a helper, 2024-02-02).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: use test-tool as unit test runner on Windows
Although the previous commit changed t/Makefile to run unit tests
alongside shell tests, the Windows CI still needs a separate unit-tests
step due to how the test sharding works.
We want to avoid using `prove` as a test running on Windows due to
performance issues [1], so use the new test-tool runner instead.
[1] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Although the previous commit changed t/Makefile to run unit tests
alongside shell tests, the Windows CI still needs a separate unit-tests
step due to how the test sharding works.
We want to avoid using `prove` as a test running on Windows due to
performance issues [1], so use the new test-tool runner instead.
[1] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
t/Makefile: run unit tests alongside shell tests
Add a wrapper script to allow `prove` to run both shell tests and unit
tests from a single invocation. This avoids issues around running prove
twice in CI, as discussed in [1].
Additionally, this moves the unit tests into the main dev workflow, so
that errors can be spotted more quickly. Accordingly, we remove the
separate unit tests step for Linux CI. (We leave the Windows CI
unit-test step as-is, because the sharding scheme there involves
selecting specific test files rather than running `make test`.)
[1] https://lore.kernel.org/git/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a wrapper script to allow `prove` to run both shell tests and unit
tests from a single invocation. This avoids issues around running prove
twice in CI, as discussed in [1].
Additionally, this moves the unit tests into the main dev workflow, so
that errors can be spotted more quickly. Accordingly, we remove the
separate unit tests step for Linux CI. (We leave the Windows CI
unit-test step as-is, because the sharding scheme there involves
selecting specific test files rather than running `make test`.)
[1] https://lore.kernel.org/git/pull.1613.git.1699894837844.gitgitgadget@gmail.com/
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
unit tests: add rule for running with test-tool
In the previous commit, we added support in test-tool for running
collections of unit tests. Now, add rules in t/Makefile for running in
this way.
This new rule can be executed from the top-level Makefile via
`make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests`, or by
setting DEFAULT_UNIT_TEST_TARGET in config.mak.
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the previous commit, we added support in test-tool for running
collections of unit tests. Now, add rules in t/Makefile for running in
this way.
This new rule can be executed from the top-level Makefile via
`make DEFAULT_UNIT_TEST_TARGET=unit-tests-test-tool unit-tests`, or by
setting DEFAULT_UNIT_TEST_TARGET in config.mak.
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
test-tool run-command testsuite: support unit tests
Teach the testsuite runner in `test-tool run-command testsuite` how to
run unit tests: if TEST_SHELL_PATH is not set, run the programs directly
from CWD, rather than defaulting to "sh" as an interpreter.
With this change, you can now use test-tool to run the unit tests:
$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite
This should be helpful on Windows to allow running tests without
requiring Perl (for `prove`), as discussed in [1] and [2].
This again breaks backwards compatibility, as it is now required to set
TEST_SHELL_PATH properly for executing shell scripts, but again, as
noted in [2], there are no longer any such invocations in our codebase.
[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teach the testsuite runner in `test-tool run-command testsuite` how to
run unit tests: if TEST_SHELL_PATH is not set, run the programs directly
from CWD, rather than defaulting to "sh" as an interpreter.
With this change, you can now use test-tool to run the unit tests:
$ make
$ cd t/unit-tests/bin
$ ../../helper/test-tool run-command testsuite
This should be helpful on Windows to allow running tests without
requiring Perl (for `prove`), as discussed in [1] and [2].
This again breaks backwards compatibility, as it is now required to set
TEST_SHELL_PATH properly for executing shell scripts, but again, as
noted in [2], there are no longer any such invocations in our codebase.
[1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2109091323150.59@tvgsbejvaqbjf.bet/
[2] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
test-tool run-command testsuite: remove hardcoded filter
`test-tool run-command testsuite` currently assumes that it will only be
running the shell test suite, and therefore filters out anything that
does not match a hardcoded pattern of "t[0-9][0-9][0-9][0-9]-*.sh".
Later in this series, we'll adapt `test-tool run-command testsuite` to
also support unit tests, which do not follow the same naming conventions
as the shell tests, so this hardcoded pattern is inconvenient.
Since `testsuite` also allows specifying patterns on the command-line,
let's just remove this pattern. As noted in [1], there are no longer any
uses of `testsuite` in our codebase, it should be OK to break backwards
compatibility in this case. We also add a new filter to avoid trying to
execute "." and "..", so that users who wish to execute every test in a
directory can do so without specifying a pattern.
[1] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
`test-tool run-command testsuite` currently assumes that it will only be
running the shell test suite, and therefore filters out anything that
does not match a hardcoded pattern of "t[0-9][0-9][0-9][0-9]-*.sh".
Later in this series, we'll adapt `test-tool run-command testsuite` to
also support unit tests, which do not follow the same naming conventions
as the shell tests, so this hardcoded pattern is inconvenient.
Since `testsuite` also allows specifying patterns on the command-line,
let's just remove this pattern. As noted in [1], there are no longer any
uses of `testsuite` in our codebase, it should be OK to break backwards
compatibility in this case. We also add a new filter to avoid trying to
execute "." and "..", so that users who wish to execute every test in a
directory can do so without specifying a pattern.
[1] https://lore.kernel.org/git/850ea42c-f103-68d5-896b-9120e2628686@gmx.de/
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
test-tool run-command testsuite: get shell from env
When running tests through `test-tool run-command testsuite`, we
currently hardcode `sh` as the command interpreter. As discussed in [1],
this is incorrect, and we should be using the shell set in
TEST_SHELL_PATH instead.
Add a shell_path field in struct testsuite so that we can pass this to
the task runner callback. If this is non-null, we'll use it as the
argv[0] of the subprocess. Otherwise, we'll just execute the test
program directly. We will use this feature in a later commit to enable
running binary executable unit tests.
However, for now when setting up the struct testsuite in testsuite(),
use the value of TEST_SHELL_PATH if it's set, otherwise keep the
original behavior by defaulting to `sh`.
[1] https://lore.kernel.org/git/20240123005913.GB835964@coredump.intra.peff.net/
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When running tests through `test-tool run-command testsuite`, we
currently hardcode `sh` as the command interpreter. As discussed in [1],
this is incorrect, and we should be using the shell set in
TEST_SHELL_PATH instead.
Add a shell_path field in struct testsuite so that we can pass this to
the task runner callback. If this is non-null, we'll use it as the
argv[0] of the subprocess. Otherwise, we'll just execute the test
program directly. We will use this feature in a later commit to enable
running binary executable unit tests.
However, for now when setting up the struct testsuite in testsuite(),
use the value of TEST_SHELL_PATH if it's set, otherwise keep the
original behavior by defaulting to `sh`.
[1] https://lore.kernel.org/git/20240123005913.GB835964@coredump.intra.peff.net/
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
t0080: turn t-basic unit test into a helper
While t/unit-tests/t-basic.c uses the unit-test framework added in
e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is
not a true unit test in that it intentionally fails in order to exercise
various codepaths in the unit-test framework. Thus, we intentionally
exclude it when running unit tests through the various t/Makefile
targets. Instead, it is executed by t0080-unit-test-output.sh, which
verifies its output follows the TAP format expected for the various
pass, skip, or fail cases.
As such, it makes more sense for t-basic to be a helper item for
t0080-unit-test-output.sh, so let's move it to
t/helper/test-example-tap.c and adjust Makefiles as necessary.
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
While t/unit-tests/t-basic.c uses the unit-test framework added in
e137fe3b29 (unit tests: add TAP unit test framework, 2023-11-09), it is
not a true unit test in that it intentionally fails in order to exercise
various codepaths in the unit-test framework. Thus, we intentionally
exclude it when running unit tests through the various t/Makefile
targets. Instead, it is executed by t0080-unit-test-output.sh, which
verifies its output follows the TAP format expected for the various
pass, skip, or fail cases.
As such, it makes more sense for t-basic to be a helper item for
t0080-unit-test-output.sh, so let's move it to
t/helper/test-example-tap.c and adjust Makefiles as necessary.
Signed-off-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: fix Python dependency on Ubuntu 24.04
Newer versions of Ubuntu have dropped Python 2 starting with Ubuntu
23.04. By default though, our CI setups will try to use that Python
version on all Ubuntu-based jobs except for the "linux-gcc" one.
We didn't notice this issue due to two reasons:
- The "ubuntu:latest" tag always points to the latest LTS release.
Until a few weeks ago this was Ubuntu 22.04, which still had Python
2.
- Our Docker-based CI jobs had their own script to install
dependencies until 9cdeb34b96 (ci: merge scripts which install
dependencies, 2024-04-12), where we didn't even try to install
Python at all for many of them.
Since the CI refactorings have originally been implemented, Ubuntu
24.04 was released, and it being an LTS versions means that the "latest"
tag now points to that Python-2-less version. Consequently, those jobs
that use "ubuntu:latest" broke.
Address this by using Python 2 on Ubuntu 20.04, only, whereas we use
Python 3 on all other Ubuntu jobs. Eventually, we should think about
dropping support for Python 2 completely.
Reported-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Newer versions of Ubuntu have dropped Python 2 starting with Ubuntu
23.04. By default though, our CI setups will try to use that Python
version on all Ubuntu-based jobs except for the "linux-gcc" one.
We didn't notice this issue due to two reasons:
- The "ubuntu:latest" tag always points to the latest LTS release.
Until a few weeks ago this was Ubuntu 22.04, which still had Python
2.
- Our Docker-based CI jobs had their own script to install
dependencies until 9cdeb34b96 (ci: merge scripts which install
dependencies, 2024-04-12), where we didn't even try to install
Python at all for many of them.
Since the CI refactorings have originally been implemented, Ubuntu
24.04 was released, and it being an LTS versions means that the "latest"
tag now points to that Python-2-less version. Consequently, those jobs
that use "ubuntu:latest" broke.
Address this by using Python 2 on Ubuntu 20.04, only, whereas we use
Python 3 on all other Ubuntu jobs. Eventually, we should think about
dropping support for Python 2 completely.
Reported-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Documentation: Mention that refspecs are explained elsewhere
The syntax for refspecs are explained in more detail in documention for
git-fetch and git-push. Give a hint to the user too look there more fore
information
Signed-off-by: Øystein Walle <oystwa@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The syntax for refspecs are explained in more detail in documention for
git-fetch and git-push. Give a hint to the user too look there more fore
information
Signed-off-by: Øystein Walle <oystwa@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
format-patch: run range-diff with larger creation-factor
We see too often that a range-diff added to format-patch output
shows too many "unmatched" patches. This is because the default
value for creation-factor is set to a relatively low value.
It may be justified for other uses (like you have a yet-to-be-sent
new iteration of your series, and compare it against the 'seen'
branch that has an older iteration, probably with the '--left-only'
option, to pick out only your patches while ignoring the others) of
"range-diff" command, but when the command is run as part of the
format-patch, the user _knows_ and expects that the patches in the
old and the new iterations roughly correspond to each other, so we
can and should use a much higher default.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We see too often that a range-diff added to format-patch output
shows too many "unmatched" patches. This is because the default
value for creation-factor is set to a relatively low value.
It may be justified for other uses (like you have a yet-to-be-sent
new iteration of your series, and compare it against the 'seen'
branch that has an older iteration, probably with the '--left-only'
option, to pick out only your patches while ignoring the others) of
"range-diff" command, but when the command is run as part of the
format-patch, the user _knows_ and expects that the patches in the
old and the new iterations roughly correspond to each other, so we
can and should use a much higher default.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitlab-ci: add smoke test for fuzzers
Our GitLab CI setup has a test gap where the fuzzers aren't exercised at
all. Add a smoke test, similar to the one we have in GitHub Workflows.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Our GitLab CI setup has a test gap where the fuzzers aren't exercised at
all. Add a smoke test, similar to the one we have in GitHub Workflows.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: display subcommand help
Until now, `git config -h` would have printed help for the old-style
syntax. Now that all modes have proper subcommands though it is
preferable to instead display the subcommand help.
Drop the `NO_INTERNAL_HELP` flag to do so. While at it, drop the help
mismatch in t0450 and add the `--get-colorbool` option to the usage such
that git-config(1)'s synopsis and `git config -h` match.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Until now, `git config -h` would have printed help for the old-style
syntax. Now that all modes have proper subcommands though it is
preferable to instead display the subcommand help.
Drop the `NO_INTERNAL_HELP` flag to do so. While at it, drop the help
mismatch in t0450 and add the `--get-colorbool` option to the usage such
that git-config(1)'s synopsis and `git config -h` match.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: introduce "edit" subcommand
Introduce a new "edit" subcommand to git-config(1). Please refer to
preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce a new "edit" subcommand to git-config(1). Please refer to
preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: introduce "remove-section" subcommand
Introduce a new "remove-section" subcommand to git-config(1). Please
refer to preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce a new "remove-section" subcommand to git-config(1). Please
refer to preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: introduce "rename-section" subcommand
Introduce a new "rename-section" subcommand to git-config(1). Please
refer to preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce a new "rename-section" subcommand to git-config(1). Please
refer to preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: introduce "unset" subcommand
Introduce a new "unset" subcommand to git-config(1). Please refer to
preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce a new "unset" subcommand to git-config(1). Please refer to
preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: introduce "set" subcommand
Introduce a new "set" subcommand to git-config(1). Please refer to
preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce a new "set" subcommand to git-config(1). Please refer to
preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: introduce "get" subcommand
Introduce a new "get" subcommand to git-config(1). Please refer to
preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce a new "get" subcommand to git-config(1). Please refer to
preceding commits regarding the motivation behind this change.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: introduce "list" subcommand
While git-config(1) has several modes, those modes are not exposed with
subcommands but instead by specifying action flags like `--unset` or
`--list`. This user interface is not really in line with how our more
modern commands work, where it is a lot more customary to say e.g. `git
remote list`. Furthermore, to add to the confusion, git-config(1) also
allows the user to request modes implicitly by just specifying the
correct number of arguments. Thus, `git config foo.bar` will retrieve
the value of "foo.bar" while `git config foo.bar baz` will set it to
"baz".
Overall, this makes for a confusing interface that could really use a
makeover. It hurts discoverability of what you can do with git-config(1)
and is comparatively easy to get wrong. Converting the command to have
subcommands instead would go a long way to help address these issues.
One concern in this context is backwards compatibility. Luckily, we can
introduce subcommands without breaking backwards compatibility at all.
This is because all the implicit modes of git-config(1) require that the
first argument is a properly formatted config key. And as config keys
_must_ have a dot in their name, any value without a dot would have been
discarded by git-config(1) previous to this change. Thus, given that
none of the subcommands do have a dot, they are unambiguous.
Introduce the first such new subcommand, which is "git config list". To
retain backwards compatibility we only conditionally use subcommands and
will fall back to the old syntax in case no subcommand was detected.
This should help to transition to the new-style syntax until we
eventually deprecate and remove the old-style syntax.
Note that the way we handle this we're duplicating some functionality
across old and new syntax. While this isn't pretty, it helps us to
ensure that there really is no change in behaviour for the old syntax.
Amend tests such that we run them both with old and new style syntax.
As tests are now run twice, state from the first run may be still be
around in the second run and thus cause tests to fail. Add cleanup logic
as required to fix such tests.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
While git-config(1) has several modes, those modes are not exposed with
subcommands but instead by specifying action flags like `--unset` or
`--list`. This user interface is not really in line with how our more
modern commands work, where it is a lot more customary to say e.g. `git
remote list`. Furthermore, to add to the confusion, git-config(1) also
allows the user to request modes implicitly by just specifying the
correct number of arguments. Thus, `git config foo.bar` will retrieve
the value of "foo.bar" while `git config foo.bar baz` will set it to
"baz".
Overall, this makes for a confusing interface that could really use a
makeover. It hurts discoverability of what you can do with git-config(1)
and is comparatively easy to get wrong. Converting the command to have
subcommands instead would go a long way to help address these issues.
One concern in this context is backwards compatibility. Luckily, we can
introduce subcommands without breaking backwards compatibility at all.
This is because all the implicit modes of git-config(1) require that the
first argument is a properly formatted config key. And as config keys
_must_ have a dot in their name, any value without a dot would have been
discarded by git-config(1) previous to this change. Thus, given that
none of the subcommands do have a dot, they are unambiguous.
Introduce the first such new subcommand, which is "git config list". To
retain backwards compatibility we only conditionally use subcommands and
will fall back to the old syntax in case no subcommand was detected.
This should help to transition to the new-style syntax until we
eventually deprecate and remove the old-style syntax.
Note that the way we handle this we're duplicating some functionality
across old and new syntax. While this isn't pretty, it helps us to
ensure that there really is no change in behaviour for the old syntax.
Amend tests such that we run them both with old and new style syntax.
As tests are now run twice, state from the first run may be still be
around in the second run and thus cause tests to fail. Add cleanup logic
as required to fix such tests.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: pull out function to handle `--null`
Pull out function to handle the `--null` option, which we are about to
reuse in subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Pull out function to handle the `--null` option, which we are about to
reuse in subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: pull out function to handle config location
There's quite a bunch of options to git-config(1) that allow the user to
specify which config location to use when reading or writing config
options. The logic to handle this is thus by necessity also quite
involved.
Pull it out into a separate function so that we can reuse it in
subsequent commits which introduce proper subcommands.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There's quite a bunch of options to git-config(1) that allow the user to
specify which config location to use when reading or writing config
options. The logic to handle this is thus by necessity also quite
involved.
Pull it out into a separate function so that we can reuse it in
subsequent commits which introduce proper subcommands.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: use `OPT_CMDMODE()` to specify modes
The git-config(1) command has various different modes which are
accessible via e.g. `--get-urlmatch` or `--unset-all`. These modes are
declared with `OPT_BIT()`, which causes two minor issues:
- The respective modes also have a negated form `--no-get-urlmatch`,
which is unintended.
- We have to manually handle exclusiveness of the modes.
Switch these options to instead use `OPT_CMDMODE()`, which is made
exactly for this usecase. Remove the now-unneeded check that only a
single mode is given, which is now handled by the parse-options
interface.
While at it, format optional placeholders for arguments to conform to
our style guidelines by using `[<placeholder>]`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The git-config(1) command has various different modes which are
accessible via e.g. `--get-urlmatch` or `--unset-all`. These modes are
declared with `OPT_BIT()`, which causes two minor issues:
- The respective modes also have a negated form `--no-get-urlmatch`,
which is unintended.
- We have to manually handle exclusiveness of the modes.
Switch these options to instead use `OPT_CMDMODE()`, which is made
exactly for this usecase. Remove the now-unneeded check that only a
single mode is given, which is now handled by the parse-options
interface.
While at it, format optional placeholders for arguments to conform to
our style guidelines by using `[<placeholder>]`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: move "fixed-value" option to correct group
The `--fixed-value` option can be used to alter how the value-pattern
parameter is interpreted for the various actions of git-config(1). But
while it is an option, it is currently listed as part of the actions
group, which is wrong.
Move the option to the "Other" group, which hosts the various options
known to git-config(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `--fixed-value` option can be used to alter how the value-pattern
parameter is interpreted for the various actions of git-config(1). But
while it is an option, it is currently listed as part of the actions
group, which is wrong.
Move the option to the "Other" group, which hosts the various options
known to git-config(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/config: move option array around
Move around the option array. This will help us with a follow-up commit
that introduces subcommands to git-config(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Move around the option array. This will help us with a follow-up commit
that introduces subcommands to git-config(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
config: clarify memory ownership when preparing comment strings
The ownership of memory returned when preparing a comment string is
quite intricate: when the returned value is different than the passed
value, then the caller is responsible to free the memory. This is quite
subtle, and it's even easier to miss because the returned value is in
fact a `const char *`.
Adapt the function to always return either `NULL` or a newly allocated
string. The function is called at most once per git-config(1), so it's
not like this micro-optimization really matters. Thus, callers are now
always responsible for freeing the value.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The ownership of memory returned when preparing a comment string is
quite intricate: when the returned value is different than the passed
value, then the caller is responsible to free the memory. This is quite
subtle, and it's even easier to miss because the returned value is in
fact a `const char *`.
Adapt the function to always return either `NULL` or a newly allocated
string. The function is called at most once per git-config(1), so it's
not like this micro-optimization really matters. Thus, callers are now
always responsible for freeing the value.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff: fix --exit-code with external diff
You can ask the diff machinery to let the exit code indicate whether
there are changes, e.g. with --exit-code. It as two ways to calculate
that bit: The quick one assumes blobs with different hashes have
different content, and the more elaborate way actually compares the
contents, possibly applying transformations like ignoring whitespace.
Always use the slower path by setting the flag diff_from_contents,
because any of the files could have an external diff driver set via an
attribute, which might consider binary differences irrelevant, like e.g.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
You can ask the diff machinery to let the exit code indicate whether
there are changes, e.g. with --exit-code. It as two ways to calculate
that bit: The quick one assumes blobs with different hashes have
different content, and the more elaborate way actually compares the
contents, possibly applying transformations like ignoring whitespace.
Always use the slower path by setting the flag diff_from_contents,
because any of the files could have an external diff driver set via an
attribute, which might consider binary differences irrelevant, like e.g.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff: report unmerged paths as changes in run_diff_cmd()
You can ask the diff machinery to let the exit code indicate whether
there are changes, e.g. with --quiet. It as two ways to calculate that
bit: The quick one assumes blobs with different hashes have different
content, and the more elaborate way actually compares the contents,
possibly applying transformations like ignoring whitespace.
The quick way considers an unmerged file to be a change and reports
exit code 1, which makes sense.
The slower path uses the struct diff_options member found_changes to
indicate whether the blobs differ even with the transformations applied.
It's not set for unmerged files, though, resulting in exit code 0.
Set found_changes in run_diff_cmd() for unmerged files, for a consistent
exit code of 1 if there's an unmerged file, regardless of whether
whitespace is ignored.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
You can ask the diff machinery to let the exit code indicate whether
there are changes, e.g. with --quiet. It as two ways to calculate that
bit: The quick one assumes blobs with different hashes have different
content, and the more elaborate way actually compares the contents,
possibly applying transformations like ignoring whitespace.
The quick way considers an unmerged file to be a change and reports
exit code 1, which makes sense.
The slower path uses the struct diff_options member found_changes to
indicate whether the blobs differ even with the transformations applied.
It's not set for unmerged files, though, resulting in exit code 0.
Set found_changes in run_diff_cmd() for unmerged files, for a consistent
exit code of 1 if there's an unmerged file, regardless of whether
whitespace is ignored.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: return conflict error when checking packed refs
The TRANSACTION_NAME_CONFLICT error code refers to a failure to create a
ref due to a name conflict with another ref. An example of this is a
directory/file conflict such as ref names A/B and A.
"git fetch" uses this error code to more accurately describe the error
by recommending to the user that they try running "git remote prune" to
remove any old refs that are deleted by the remote which would clear up
any directory/file conflicts.
This helpful error message is not displayed when the conflicted ref is
stored in packed refs. This change fixes this by ensuring error return
code consistency in `lock_raw_ref`.
Signed-off-by: Ivan Tse <ivan.tse1@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The TRANSACTION_NAME_CONFLICT error code refers to a failure to create a
ref due to a name conflict with another ref. An example of this is a
directory/file conflict such as ref names A/B and A.
"git fetch" uses this error code to more accurately describe the error
by recommending to the user that they try running "git remote prune" to
remove any old refs that are deleted by the remote which would clear up
any directory/file conflicts.
This helpful error message is not displayed when the conflicted ref is
stored in packed refs. This change fixes this by ensuring error return
code consistency in `lock_raw_ref`.
Signed-off-by: Ivan Tse <ivan.tse1@gmail.com>
Acked-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
attr.c: move ATTR_MAX_FILE_SIZE check into read_attr_from_buf()
Commit 3c50032ff52 (attr: ignore overly large gitattributes files,
2022-12-01) added a defense-in-depth check to ensure that .gitattributes
blobs read from the index do not exceed ATTR_MAX_FILE_SIZE (100 MB).
But there were two cases added shortly after 3c50032ff52 was written
which do not apply similar protections:
- 47cfc9bd7d0 (attr: add flag `--source` to work with tree-ish,
2023-01-14)
- 4723ae1007f (attr.c: read attributes in a sparse directory,
2023-08-11) added a similar
Ensure that we refuse to process a .gitattributes blob exceeding
ATTR_MAX_FILE_SIZE when reading from either an arbitrary tree object or
a sparse directory. This is done by pushing the ATTR_MAX_FILE_SIZE check
down into the low-level `read_attr_from_buf()`.
In doing so, plug a leak in `read_attr_from_index()` where we would
accidentally leak the large buffer upon detecting it is too large to
process.
(Since `read_attr_from_buf()` handles a NULL buffer input, we can remove
a NULL check before calling it in `read_attr_from_index()` as well).
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commit 3c50032ff52 (attr: ignore overly large gitattributes files,
2022-12-01) added a defense-in-depth check to ensure that .gitattributes
blobs read from the index do not exceed ATTR_MAX_FILE_SIZE (100 MB).
But there were two cases added shortly after 3c50032ff52 was written
which do not apply similar protections:
- 47cfc9bd7d0 (attr: add flag `--source` to work with tree-ish,
2023-01-14)
- 4723ae1007f (attr.c: read attributes in a sparse directory,
2023-08-11) added a similar
Ensure that we refuse to process a .gitattributes blob exceeding
ATTR_MAX_FILE_SIZE when reading from either an arbitrary tree object or
a sparse directory. This is done by pushing the ATTR_MAX_FILE_SIZE check
down into the low-level `read_attr_from_buf()`.
In doing so, plug a leak in `read_attr_from_index()` where we would
accidentally leak the large buffer upon detecting it is too large to
process.
(Since `read_attr_from_buf()` handles a NULL buffer input, we can remove
a NULL check before calling it in `read_attr_from_index()` as well).
Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
gitlab-ci: add whitespace error check
GitLab CI does not have a job to check for whitespace errors introduced
by a set of changes. Reuse the existing generic `whitespace-check.sh` to
create the job for GitLab pipelines.
Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only
available in GitLab merge request pipelines and therefore the CI job is
configured to only run as part of those pipelines.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
GitLab CI does not have a job to check for whitespace errors introduced
by a set of changes. Reuse the existing generic `whitespace-check.sh` to
create the job for GitLab pipelines.
Note that the `$CI_MERGE_REQUEST_TARGET_BRANCH_SHA` variable is only
available in GitLab merge request pipelines and therefore the CI job is
configured to only run as part of those pipelines.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: make the whitespace report optional
The `check-whitespace` CI job generates a formatted output file
containing whitespace error information. As not all CI providers support
rendering a formatted summary, make its generation optional.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `check-whitespace` CI job generates a formatted output file
containing whitespace error information. As not all CI providers support
rendering a formatted summary, make its generation optional.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: separate whitespace check script
The `check-whitespace` CI job is only available as a GitHub action. To
help enable this job with other CI providers, first separate the logic
performing the whitespace check into its own script. In subsequent
commits, this script is further generalized allowing its reuse.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `check-whitespace` CI job is only available as a GitHub action. To
help enable this job with other CI providers, first separate the logic
performing the whitespace check into its own script. In subsequent
commits, this script is further generalized allowing its reuse.
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
github-ci: fix link to whitespace error
When the `check-whitespace` CI job detects whitespace errors, a
formatted summary of the issue is generated. This summary contains links
to the commits and blobs responsible for the whitespace errors. The
generated links for blobs do not work and result in a 404.
Instead of using the reference name in the link, use the commit ID
directly. This fixes the broken link and also helps enable future
generalization of the script for other CI providers by removing one of
the GitHub specific CI variables used.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When the `check-whitespace` CI job detects whitespace errors, a
formatted summary of the issue is generated. This summary contains links
to the commits and blobs responsible for the whitespace errors. The
generated links for blobs do not work and result in a 404.
Instead of using the reference name in the link, use the commit ID
directly. This fixes the broken link and also helps enable future
generalization of the script for other CI providers by removing one of
the GitHub specific CI variables used.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: pre-collapse GitLab CI sections
Sections of CI output defined by `begin_group()` and `end_group()` are
expanded in GitLab pipelines by default. This can make CI job output
rather noisy and harder to navigate. Update the behavior for GitLab
pipelines to now collapse sections by default.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sections of CI output defined by `begin_group()` and `end_group()` are
expanded in GitLab pipelines by default. This can make CI job output
rather noisy and harder to navigate. Update the behavior for GitLab
pipelines to now collapse sections by default.
Signed-off-by: Justin Tobler <jltobler@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
advice: add --no-advice global option
Advice hints must be disabled individually by setting the relevant
advice.* variables to false in the Git configuration. For server-side
and scripted usages of Git where hints can be a hindrance, it can be
cumbersome to maintain configuration to ensure all advice hints are
disabled in perpetuity. This is a particular concern in tests, where
new or changed hints can result in failed assertions.
Add a --no-advice global option to disable all advice hints from being
displayed. This is independent of the toggles for individual advice
hints. Use an internal environment variable (GIT_ADVICE) to ensure this
configuration is propagated to the usage site, even if it executes in a
subprocess.
Signed-off-by: James Liu <james@jamesliu.io>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Advice hints must be disabled individually by setting the relevant
advice.* variables to false in the Git configuration. For server-side
and scripted usages of Git where hints can be a hindrance, it can be
cumbersome to maintain configuration to ensure all advice hints are
disabled in perpetuity. This is a particular concern in tests, where
new or changed hints can result in failed assertions.
Add a --no-advice global option to disable all advice hints from being
displayed. This is independent of the toggles for individual advice
hints. Use an internal environment variable (GIT_ADVICE) to ensure this
configuration is propagated to the usage site, even if it executes in a
subprocess.
Signed-off-by: James Liu <james@jamesliu.io>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
doc: add spacing around paginate options
Make the documentation page consistent with the usage string printed by
"git help git" and consistent with the description of "[-v | --version]"
option.
Signed-off-by: James Liu <james@jamesliu.io>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Make the documentation page consistent with the usage string printed by
"git help git" and consistent with the description of "[-v | --version]"
option.
Signed-off-by: James Liu <james@jamesliu.io>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
doc: clean up usage documentation for --no-* opts
We'll be adding another option to the --no-* class of options soon.
Clean up the existing options by grouping them together in the OPTIONS
section, and adding missing ones to the SYNOPSIS.
Signed-off-by: James Liu <james@jamesliu.io>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We'll be adding another option to the --no-* class of options soon.
Clean up the existing options by grouping them together in the OPTIONS
section, and adding missing ones to the SYNOPSIS.
Signed-off-by: James Liu <james@jamesliu.io>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
stop using HEAD for attributes in bare repository by default
With 23865355 (attr: read attributes from HEAD when bare repo,
2023-10-13), we started to use the HEAD tree as the default
attribute source in a bare repository. One argument for such a
behaviour is that it would make things like "git archive" run in
bare and non-bare repositories for the same commit consistent.
This changes was merged to Git 2.43 but without an explicit mention
in its release notes.
It turns out that this change destroys performance of shallowly
cloning from a bare repository. As the "server" installations are
expected to be mostly bare, and "git pack-objects", which is the
core of driving the other side of "git clone" and "git fetch" wants
to see if a path is set not to delta with blobs from other paths via
the attribute system, the change forces the server side to traverse
the tree of the HEAD commit needlessly to find if each and every
paths the objects it sends out has the attribute that controls the
deltification. Given that (1) most projects do not configure such
an attribute, and (2) it is dubious for the server side to honor
such an end-user supplied attribute anyway, this was a poor choice
of the default.
To mitigate the current situation, let's revert the change that uses
the tree of HEAD in a bare repository by default as the attribute
source. This will help most people who have been happy with the
behaviour of Git 2.42 and before.
Two things to note:
* If you are stuck with versions of Git 2.43 or newer, that is
older than the release this fix appears in, you can explicitly
set the attr.tree configuration variable to point at an empty
tree object, i.e.
$ git config attr.tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
* If you like the behaviour we are reverting, you can explicitly
set the attr.tree configuration variable to HEAD, i.e.
$ git config attr.tree HEAD
The right fix for this is to optimize the code paths that allow
accesses to attributes in tree objects, but that is a much more
involved change and is left as a longer-term project, outside the
scope of this "first step" fix.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
With 23865355 (attr: read attributes from HEAD when bare repo,
2023-10-13), we started to use the HEAD tree as the default
attribute source in a bare repository. One argument for such a
behaviour is that it would make things like "git archive" run in
bare and non-bare repositories for the same commit consistent.
This changes was merged to Git 2.43 but without an explicit mention
in its release notes.
It turns out that this change destroys performance of shallowly
cloning from a bare repository. As the "server" installations are
expected to be mostly bare, and "git pack-objects", which is the
core of driving the other side of "git clone" and "git fetch" wants
to see if a path is set not to delta with blobs from other paths via
the attribute system, the change forces the server side to traverse
the tree of the HEAD commit needlessly to find if each and every
paths the objects it sends out has the attribute that controls the
deltification. Given that (1) most projects do not configure such
an attribute, and (2) it is dubious for the server side to honor
such an end-user supplied attribute anyway, this was a poor choice
of the default.
To mitigate the current situation, let's revert the change that uses
the tree of HEAD in a bare repository by default as the attribute
source. This will help most people who have been happy with the
behaviour of Git 2.42 and before.
Two things to note:
* If you are stuck with versions of Git 2.43 or newer, that is
older than the release this fix appears in, you can explicitly
set the attr.tree configuration variable to point at an empty
tree object, i.e.
$ git config attr.tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
* If you like the behaviour we are reverting, you can explicitly
set the attr.tree configuration variable to HEAD, i.e.
$ git config attr.tree HEAD
The right fix for this is to optimize the code paths that allow
accesses to attributes in tree objects, but that is a much more
involved change and is left as a longer-term project, outside the
scope of this "first step" fix.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
win32: fix building with NO_UNIX_SOCKETS
After 2406bf5f (Win32: detect unix socket support at runtime,
2024-04-03), it fails with:
compat/mingw.c:4160:5: error: no previous prototype for function 'mingw_have_unix_sockets' [-Werror,-Wmissing-prototypes]
4160 | int mingw_have_unix_sockets(void)
| ^
because the prototype is behind `ifndef NO_UNIX_SOCKETS`.
Signed-off-by: Mike Hommey <mh@glandium.org>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
After 2406bf5f (Win32: detect unix socket support at runtime,
2024-04-03), it fails with:
compat/mingw.c:4160:5: error: no previous prototype for function 'mingw_have_unix_sockets' [-Werror,-Wmissing-prototypes]
4160 | int mingw_have_unix_sockets(void)
| ^
because the prototype is behind `ifndef NO_UNIX_SOCKETS`.
Signed-off-by: Mike Hommey <mh@glandium.org>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
t/lib-chunk: work around broken "mv" on some vintage of macOS
When the destination is read-only, "mv" on some version of macOS
asks whether to replace the destination even though in the test its
stdin is not a terminal (and thus doesn't conform to POSIX[1]).
The helper to corrupt a chunk-file is designed to work on the
files like commit-graph and multi-pack-index files that are
generally read-only, so use "mv -f" to work around this issue.
Helped-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When the destination is read-only, "mv" on some version of macOS
asks whether to replace the destination even though in the test its
stdin is not a terminal (and thus doesn't conform to POSIX[1]).
The helper to corrupt a chunk-file is designed to work on the
files like commit-graph and multi-pack-index files that are
generally read-only, so use "mv -f" to work around this issue.
Helped-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
trailer unit tests: inspect iterator contents
Previously we only checked whether we would iterate a certain (expected)
number of times.
Also check the parsed "raw", "key" and "val" fields during each
iteration.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Previously we only checked whether we would iterate a certain (expected)
number of times.
Also check the parsed "raw", "key" and "val" fields during each
iteration.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
trailer: document parse_trailers() usage
Explain how to use parse_trailers(), because earlier we made the
trailer_info struct opaque. That is, because clients can no longer peek
inside it, we should give them guidance about how the (pointer to the)
opaque struct can still be useful to them.
Rename "head" struct to "trailer_objects" to make the wording of the new
comments a bit easier to read (because "head" itself doesn't really have
any domain-specific meaning here).
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Explain how to use parse_trailers(), because earlier we made the
trailer_info struct opaque. That is, because clients can no longer peek
inside it, we should give them guidance about how the (pointer to the)
opaque struct can still be useful to them.
Rename "head" struct to "trailer_objects" to make the wording of the new
comments a bit easier to read (because "head" itself doesn't really have
any domain-specific meaning here).
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
trailer: retire trailer_info_get() from API
Make trailer_info_get() "static" to be file-scoped to trailer.c, because
no one outside of trailer.c uses it. Remove its declaration from
<trailer.h>.
We have to also reposition it to be above parse_trailers(), which
depends on it.
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Make trailer_info_get() "static" to be file-scoped to trailer.c, because
no one outside of trailer.c uses it. Remove its declaration from
<trailer.h>.
We have to also reposition it to be above parse_trailers(), which
depends on it.
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
trailer: make trailer_info struct private
In 13211ae23f (trailer: separate public from internal portion of
trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous
struct to discourage use by trailer.h API users. However it still left
open the possibility of external use of trailer_info itself. Now that
there are no external users of trailer_info, we can make this struct
private.
Make this struct private by putting its definition inside trailer.c.
This has two benefits:
(1) it makes the surface area of the public facing
interface (trailer.h) smaller, and
(2) external API users are unable to peer inside this struct (because
it is only ever exposed as an opaque pointer).
There are a few disadvantages:
(A) every time the member of the struct is accessed an extra pointer
dereference must be done, and
(B) for users of trailer_info outside trailer.c, this struct can no
longer be allocated on the stack and may only be allocated on the
heap (because its definition is hidden away in trailer.c) and
appropriately deallocated by the user, and
(C) without good documentation on the API, the opaque struct is
hostile to programmers by going opposite to the "Show me your
data structures, and I won't usually need your code; it'll
be obvious." mantra [2].
(The disadvantages have already been observed in the two preparatory
commits that precede this one.) This commit believes that the benefits
outweigh the disadvantages for designing APIs, as explained below.
Making trailer_info private exposes existing deficiencies in the API.
This is because users of this struct had full access to its internals,
so there wasn't much need to actually design it to be "complete" in the
sense that API users only needed to use what was provided by the API.
For example, the location of the trailer block (start/end offsets
relative to the start of the input text) was accessible by looking at
these struct members directly. Now that the struct is private, we have
to expose new API functions to allow clients to access this
information (see builtin/interpret-trailers.c).
The idea in this commit to hide implementation details behind an "opaque
pointer" is also known as the "pimpl" (pointer to implementation) idiom
in C++ and is a common pattern in that language (where, for example,
abstract classes only have pointers to concrete classes).
However, the original inspiration to use this idiom does not come from
C++, but instead the book "C Interfaces and Implementations: Techniques
for Creating Reusable Software" [1]. This book recommends opaque
pointers as a good design principle for designing C libraries, using the
term "interface" as the functions defined in *.h (header) files and
"implementation" as the corresponding *.c file which define the
interfaces.
The book says this about opaque pointers:
... clients can manipulate such pointers freely, but they can’t
dereference them; that is, they can’t look at the innards of the
structure pointed to by them. Only the implementation has that
privilege. Opaque pointers hide representation details and help
catch errors.
In our case, "struct trailer_info" is now hidden from clients, and the
ways in which this opaque pointer can be used is limited to the richness
of <trailer.h>. In other words, <trailer.h> exclusively controls exactly
how "trailer_info" pointers are to be used.
[1] Hanson, David R. "C Interfaces and Implementations: Techniques for
Creating Reusable Software". Addison Wesley, 1997. p. 22
[2] Raymond, Eric S. "The Cathedral and the Bazaar: Musings on Linux and
Open Source by an Accidental Revolutionary". O'Reilly, 1999.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In 13211ae23f (trailer: separate public from internal portion of
trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous
struct to discourage use by trailer.h API users. However it still left
open the possibility of external use of trailer_info itself. Now that
there are no external users of trailer_info, we can make this struct
private.
Make this struct private by putting its definition inside trailer.c.
This has two benefits:
(1) it makes the surface area of the public facing
interface (trailer.h) smaller, and
(2) external API users are unable to peer inside this struct (because
it is only ever exposed as an opaque pointer).
There are a few disadvantages:
(A) every time the member of the struct is accessed an extra pointer
dereference must be done, and
(B) for users of trailer_info outside trailer.c, this struct can no
longer be allocated on the stack and may only be allocated on the
heap (because its definition is hidden away in trailer.c) and
appropriately deallocated by the user, and
(C) without good documentation on the API, the opaque struct is
hostile to programmers by going opposite to the "Show me your
data structures, and I won't usually need your code; it'll
be obvious." mantra [2].
(The disadvantages have already been observed in the two preparatory
commits that precede this one.) This commit believes that the benefits
outweigh the disadvantages for designing APIs, as explained below.
Making trailer_info private exposes existing deficiencies in the API.
This is because users of this struct had full access to its internals,
so there wasn't much need to actually design it to be "complete" in the
sense that API users only needed to use what was provided by the API.
For example, the location of the trailer block (start/end offsets
relative to the start of the input text) was accessible by looking at
these struct members directly. Now that the struct is private, we have
to expose new API functions to allow clients to access this
information (see builtin/interpret-trailers.c).
The idea in this commit to hide implementation details behind an "opaque
pointer" is also known as the "pimpl" (pointer to implementation) idiom
in C++ and is a common pattern in that language (where, for example,
abstract classes only have pointers to concrete classes).
However, the original inspiration to use this idiom does not come from
C++, but instead the book "C Interfaces and Implementations: Techniques
for Creating Reusable Software" [1]. This book recommends opaque
pointers as a good design principle for designing C libraries, using the
term "interface" as the functions defined in *.h (header) files and
"implementation" as the corresponding *.c file which define the
interfaces.
The book says this about opaque pointers:
... clients can manipulate such pointers freely, but they can’t
dereference them; that is, they can’t look at the innards of the
structure pointed to by them. Only the implementation has that
privilege. Opaque pointers hide representation details and help
catch errors.
In our case, "struct trailer_info" is now hidden from clients, and the
ways in which this opaque pointer can be used is limited to the richness
of <trailer.h>. In other words, <trailer.h> exclusively controls exactly
how "trailer_info" pointers are to be used.
[1] Hanson, David R. "C Interfaces and Implementations: Techniques for
Creating Reusable Software". Addison Wesley, 1997. p. 22
[2] Raymond, Eric S. "The Cathedral and the Bazaar: Musings on Linux and
Open Source by an Accidental Revolutionary". O'Reilly, 1999.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
trailer: make parse_trailers() return trailer_info pointer
This is the second and final preparatory commit for making the
trailer_info struct private to the trailer implementation.
Make trailer_info_get() do the actual work of allocating a new
trailer_info struct, and return a pointer to it. Because
parse_trailers() wraps around trailer_info_get(), it too can return this
pointer to the caller. From the trailer API user's perspective, the call
to trailer_info_new() can be replaced with parse_trailers(); do so in
interpret-trailers.
Because trailer_info_new() is no longer called by interpret-trailers,
remove this function from the trailer API.
With this change, we no longer allocate trailer_info on the stack ---
all uses of it are via a pointer where the actual data is always
allocated at runtime through trailer_info_new(). Make
trailer_info_release() free this dynamically allocated memory.
Finally, due to the way the function signatures of parse_trailers() and
trailer_info_get() have changed, update the callsites in
format_trailers_from_commit() and trailer_iterator_init() accordingly.
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is the second and final preparatory commit for making the
trailer_info struct private to the trailer implementation.
Make trailer_info_get() do the actual work of allocating a new
trailer_info struct, and return a pointer to it. Because
parse_trailers() wraps around trailer_info_get(), it too can return this
pointer to the caller. From the trailer API user's perspective, the call
to trailer_info_new() can be replaced with parse_trailers(); do so in
interpret-trailers.
Because trailer_info_new() is no longer called by interpret-trailers,
remove this function from the trailer API.
With this change, we no longer allocate trailer_info on the stack ---
all uses of it are via a pointer where the actual data is always
allocated at runtime through trailer_info_new(). Make
trailer_info_release() free this dynamically allocated memory.
Finally, due to the way the function signatures of parse_trailers() and
trailer_info_get() have changed, update the callsites in
format_trailers_from_commit() and trailer_iterator_init() accordingly.
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
interpret-trailers: access trailer_info with new helpers
Instead of directly accessing trailer_info members, access them
indirectly through new helper functions exposed by the trailer API.
This is the first of two preparatory commits which will allow us to
use the so-called "pimpl" (pointer to implementation) idiom for the
trailer API, by making the trailer_info struct private to the trailer
implementation (and thus hidden from the API).
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of directly accessing trailer_info members, access them
indirectly through new helper functions exposed by the trailer API.
This is the first of two preparatory commits which will allow us to
use the so-called "pimpl" (pointer to implementation) idiom for the
trailer API, by making the trailer_info struct private to the trailer
implementation (and thus hidden from the API).
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
sequencer: use the trailer iterator
Instead of calling "trailer_info_get()", which is a low-level function
in the trailers implementation (trailer.c), call
trailer_iterator_advance(), which was specifically designed for public
consumption in f0939a0eb1 (trailer: add interface for iterating over
commit trailers, 2020-09-27).
Avoiding "trailer_info_get()" means we don't have to worry about options
like "no_divider" (relevant for parsing trailers). We also don't have to
check for things like "info.trailer_start == info.trailer_end" to see
whether there were any trailers (instead we can just check to see
whether the iterator advanced at all).
Note how we have to use "iter.raw" in order to get the same behavior as
before when we iterated over the unparsed string array (char **trailers)
in trailer_info.
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of calling "trailer_info_get()", which is a low-level function
in the trailers implementation (trailer.c), call
trailer_iterator_advance(), which was specifically designed for public
consumption in f0939a0eb1 (trailer: add interface for iterating over
commit trailers, 2020-09-27).
Avoiding "trailer_info_get()" means we don't have to worry about options
like "no_divider" (relevant for parsing trailers). We also don't have to
check for things like "info.trailer_start == info.trailer_end" to see
whether there were any trailers (instead we can just check to see
whether the iterator advanced at all).
Note how we have to use "iter.raw" in order to get the same behavior as
before when we iterated over the unparsed string array (char **trailers)
in trailer_info.
Signed-off-by: Linus Arver <linus@ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>