There are double frees and leaks around setup_revisions() API used
in "git stash show", which has been fixed, and setup_revisions()
API gained a wrapper to make it more ergonomic when using it with
strvec-manged argc/argv pairs.
* jk/setup-revisions-freefix:
revision: retain argv NULL invariant in setup_revisions()
treewide: pass strvecs around for setup_revisions_from_strvec()
treewide: use setup_revisions_from_strvec() when we have a strvec
revision: add wrapper to setup_revisions() from a strvec
revision: manage memory ownership of argv in setup_revisions()
stash: tell setup_revisions() to free our allocated strings
"git rebase -i" failed to clean-up the commit log message when the
command commits the final one in a chain of "fixup" commands, which
has been corrected.
* pw/rebase-i-cleanup-fix:
sequencer: remove VERBATIM_MSG flag
rebase -i: respect commit.cleanup when picking fixups
Keep giving hint about the default initial branch name for users
who may be surprised after Git 3.0 switch-over.
* jc/3.0-default-initial-branch-to-main-addendum:
initial branch: give hints after switching the default name
Declare that "git init" that is not otherwise configured uses
'main' as the initial branch, not 'master', starting Git 3.0.
* pw/3.0-default-initial-branch-to-main:
t0613: stop setting default initial branch
t9902: switch default branch name to main
t4013: switch default branch name to main
breaking-changes: switch default branch to main
"git send-email --compose --reply-to=<address>" used to add
duplicated Reply-To: header, which made mailservers unhappy. This
has been corrected.
* nb/send-email-no-dup-reply-to:
send-email: don't duplicate Reply-to: in intro message
"git subtree" (in contrib/) did not work correctly when splitting
squashed subtrees, which has been improved.
* cs/subtree-squash-split-fix:
contrib/subtree: fix split with squashed subtrees
Some among "git add -p" and friends ignored color.diff and/or
color.ui configuration variables, which is an old regression, which
has been corrected.
* jk/add-i-color:
contrib/diff-highlight: mention interactive.diffFilter
add-interactive: manually fall back color config to color.ui
add-interactive: respect color.diff for diff coloring
stash: pass --no-color to diff plumbing child processes
The "promisor-remote" capability mechanism has been updated to
allow the "partialCloneFilter" settings and the "token" value to be
communicated from the server side.
* cc/promisor-remote-capability:
promisor-remote: use string_list_split() in mark_remotes_as_accepted()
promisor-remote: allow a client to check fields
promisor-remote: use string_list_split() in filter_promisor_remote()
promisor-remote: refactor how we parse advertised fields
promisor-remote: use string constants for 'name' and 'url' too
promisor-remote: allow a server to advertise more fields
promisor-remote: refactor to get rid of 'struct strvec'
In an argc/argv pair, the entry for argv[argc] is generally NULL. You
can iterate by counting up to argc, or by looking for the NULL entry in
argv.
When we pass such a pair to setup_revisions(), it shrinks argc to
account for the options we consumed and returns the result to the
caller. But it doesn't touch the entries after the reduced argc. So
argv[argc] will be left pointing at some arbitrary entry rather than
NULL.
This isn't the source of any known bugs, since all callers are aware of
the limitation and act accordingly. But it's a possible gotcha that may
be easy to miss.
Let's set the new argv[argc] to NULL, taking care to free it if the
caller asked us to do so.
It is tempting to do likewise for all of the entries afterwards, too, as
some of them may also need to be freed (e.g., if coming from a strvec).
But doing so isn't entirely trivial, as we munge argc in the function
(e.g., when we find "--" and move all of the entries after it into the
prune_data list). It would be possible with some light refactoring, but
it's probably not worth it. Nobody should ever look at them (they are
beyond the revised argc and past the NULL argv entry) outside of strvec
cleanup, and setup_revisions_from_strvec() already handles this case.
There's one other interesting gotcha: many callers which do not want to
provide arguments just pass 0/NULL for argc/argv. We need to check for
this case before assigning the final NULL.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The previous commit converted callers of setup_revisions() with a strvec
to use the safer and easier _from_strvec() variant.
Let's now convert spots that don't directly have a strvec, but receive
an argc/argv pair that eventually comes from one. We'll instead pass the
strvec down to the point where we call setup_revisions().
That makes these functions slightly less flexible if they were to grow
other callers that don't use strvecs, but this rigidity is buying us
some safety. It is only safe to pass the free_removed_argv_elements
option to setup_revisions() if we know the elements of argv/argc are
allocated on the heap. That isn't communicated in the type system when
we are passed the bare elements. But if we get a strvec, we know that
the elements are allocated strings.
And at any rate, each of these modified functions has only a single
caller (that has a strvec), so the loss of flexibility is unlikely to
ever matter.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The previous commit introduced a wrapper to make using setup_revisions()
with a strvec easier and safer. It converted spots that were already
doing most of what the wrapper did.
Let's now convert spots where we were not setting up the
free_removed_argv_elements flag. As discussed in the previous commit,
this probably isn't fixing any bugs or leaks (since these sites wouldn't
trigger the re-shuffling of argv that causes them). This is mostly
future-proofing us against setup_revisions() becoming more aggressive
about its re-shuffling.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The setup_revisions() function was designed to take the argc/argv pair
from the operating system. But we sometimes construct our own argv using
a strvec and pass that in. There are a few gotchas that callers need to
deal with here:
1. You should always pass the free_removed_argv_elements option via
setup_revision_opt. Otherwise, entries may be leaked if
setup_revisions() re-shuffles options.
2. After setup_revisions() returns, the strvec state is odd. We get a
reduced argc from setup_revisions() telling us how many unknown
options were left in place. Entries after that in argv may be
retained, or may be NULL (depending on how the reshuffling
happened). But the strvec's "nr" field still represents the
original value, and some of the entries it thinks it is still
storing may be NULL. Callers must be careful with how they access
it.
Some callers deal with (1), but not all. In practice they are OK because
they do not pass any options that would cause setup_revisions() to
re-shuffle (namely unknown options which may be relayed from the user,
and the use of the "--" separator). But it's probably a good idea to
consistently pass this option anyway to future-proof ourselves against
the details of setup_revisions() changing.
No callers address (2), though I don't think there any visible bugs.
Most of them simply call strvec_clear() and never otherwise look at the
result. And in fact, if they naively set foo.nr to the argc returned by
setup_revisions(), that would cause leaks! Because setup_revisions()
does not free consumed options[1], we have to leave the "nr" field of
the strvec at its original value to find and free them during
strvec_clear().
So I don't think there are any bugs to fix here, but we can make things
safer and simpler for callers. Let's introduce a helper function that
sets the free_removed_argv_elements automatically and shrinks the strvec
to represent the retained options afterwards (taking care to free the
now-obsolete entries).
We'll start by converting all of the call-sites which use the
free_removed_argv_elements option. There should be no behavior change
for them, except that their "shrunken" entries are cleaned up
immediately, rather than waiting for a strvec_clear() call.
[1] Arguably setup_revisions() should be doing this step for us if we
told it to free removed options, but there are many existing callers
which will be broken if it did. Introducing this helper is a
possible first step towards that.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The setup_revisions() function takes an argc/argv pair and consumes
arguments from it, returning a reduced argc count to the caller. But it
may also overwrite entries within the argv array, as it shifts unknown
options to the front of argv (so they can be found in the range of
0..argc-1 after we return).
For a normal argc/argv coming from the operating system, this is OK.
We don't need to worry about memory ownership of the strings in those
entries. But some callers pass in allocated strings from a strvec, and
we do need to care about those.
We faced a similar issue in f92dbdbc6a (revisions API: don't leak memory
on argv elements that need free()-ing, 2022-08-02), which added an
option for callers to tell us that elements need to be freed. But the
implementation within setup_revisions() was incomplete. It only covered
the case of dropping "--", but not the movement of unknown options.
When we shift argv entries around, we should free the elements we are
about to overwrite, so they are not leaked. For example, in:
git stash show -p --invalid
we will pass this to setup_revisions():
argc = 3, argv[] = { "show", "-p", "--invalid", NULL }
which will then return:
argc = 2, argv[] = { "show", "--invalid", "--invalid", NULL }
overwriting the "-p" entry, which is leaked unless we free it at that
moment.
You can see in the output above another potential problem. We now have
two copies of the "--invalid" string. If the caller does not respect the
new argc when free-ing the strings via strvec_clear(), we'll get a
double-free. And git-stash suffers from this, and will crash with the
above command.
So it seems at first glance that the solution is to just assign the
reduced argc to the strvec.nr field in the caller. Then it would stop
after freeing only any copied entries. But that's not always right
either!
Remember that we are reducing "argc" to account for elements we've
consumed. So if there isn't an invalid option, we'd turn:
argc = 2, argv[] = { "show", "-p", NULL }
into:
argc = 1, argv[] = { "show", "-p", NULL }
In that case strvec_clear() must keep looking past the shortened argc we
return to find the original "-p" to free. It needs to use the original
argc to do that.
We can solve this by turning our argv writes into strict moves, not
copies. When we shuffle an unknown option to the front, we'll overwrite
its old position with NULL. That leaves an argv array that may have NULL
"holes" in it.
So in the "--invalid" example above we get:
argc = 2, argv[] = { "show", "--invalid", NULL, NULL }
but something like "git stash -p --invalid -p" would yield:
argc = 3, argv[] = { "show", "--invalid", NULL, "-p", NULL }
because we move "--invalid" to overwrite the first "-p", but the second
one is quietly consumed. But strvec_clear() can handle that fine (it
iterates over the "nr" field, and passing NULL to free() is OK).
To ease the implementation, I've introduced a helper function. It's a
little hacky because it must take a double-pointer to set the old
position to NULL. Which in turn means we cannot pass "&arg", our local
alias for the current entry we're parsing, but instead "&argv[i]", the
pointer in the original array. And to make it even more confusing, we
delegate some of this work to handle_revision_opt(), which is passed a
subset of the argv array, so is always working on "&argv[0]".
Likewise, because handle_revision_opt() only receives the part of argv
left to parse, it receives the array to accumulate unknown options as a
separate unkc/unkv pair. But we're always working on the same argv
array, so our strategy works fine. I suspect this would be a bit more
obvious (and avoid some pointer cleverness) if all functions saw the
full argv array and worked with positions within it (and our new helper
would take two positions, a src and dst). But that would involve
refactoring handle_revision_opt(). I punted on that, as what's here is
not too ugly and is all contained within revision.c itself.
The new test demonstrates that "git stash show -p --invalid" no longer
crashes with a double-free (because we move instead of copy). And it
passes with SANITIZE=leak because we free "-p" before overwriting.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In "git stash show", we do a first pass of parsing our command line
options by splitting them into revision args and stash args. These are
stored in strvecs, and we pass the revision args to setup_revisions().
But setup_revisions() may modify the argv we pass it, causing us to leak
some of the entries. In particular, if it sees a "--" string, that will
be dropped from argv. This is the same as other cases addressed by
f92dbdbc6a (revisions API: don't leak memory on argv elements that need
free()-ing, 2022-08-02), and we should fix it the same way: by passing
the free_removed_argv_elements option to setup_revisions().
The added test here is run only with SANITIZE=leak, without checking its
output, because the behavior of stash with "--" is a little odd:
1. Running "git stash show" will show --stat output. But running "git
stash show --" will show --patch.
2. I'd expect a non-option after "--" to be treated as a pathspec, so:
git stash show -p 1 -- foo
would look treat "1" as a stash (a synonym for stash@{1}) and
restrict the resulting diff to "foo". But it doesn't. We split the
revision/stash args without any regard to "--". So in the example
above both "1" and "foo" are stashes. Which is an error, but also:
git stash show -- foo
treats "foo" as a stash, not a pathspec.
These are both oddities that we may want to address (or may not, if we
want to retain historical quirks). But they are well outside the scope
of this patch. So for now we'll just let the tests confirm we aren't
leaking without otherwise expecting any behavior. If we later address
either of those points and end up with another test that covers "stash
show --", we can drop this leak-only test.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Update to 10e96bc (Merge pull request #127 from
pks-gitlab/pks-ci-improvements, 2025-09-22). This commit includes a
couple of changes:
- The GitHub CI has been updated to include a 32 bit CI job.
Furthermore, the jobs now compile with "-Werror" and more warnings
enabled.
- An issue was addressed where `uintptr_t` is not available on
NonStop [1].
- The clar selftests have been restructured so that it is now possible
to add small test suites more readily. This was done to add tests
for the above addressed issue, where we now use "%p" to print
pointers in a platform dependent way.
- An issue was addressed where the test output had a trailing
whitespace with certain output formats, which caused whitespace
issues in the test expectation files.
[1]: <01c101dc2842$38903640$a9b0a2c0$@nexbridge.com>
Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
It is likely that those who came to Git after 3.0 switched the
default initial branch name to 'main' would still try to follow
tutorials that were written before 3.0 happened and with the
assumption that the tool would call the initial branch 'master'.
To help these new users after 3.0 boundary, let's retain one part of
the hint we will be giving before the default changes, namely, how
to rename the branch an unconfigured Git has created just once.
We do this without telling them how to permanently configure the
default name of the initial branch, and that design choice is very
much deliberate. The whole point of switching the default name was
because we did not want to force individual users to configure their
default branch name but while the hard wired default was 'master',
they _had_ to configure it away from 'master' in order to conform to
the recent norm, and a hint that tells them how to do so is useful.
But once the default is renamed to 'main', that no longer is true.
A narrower audience who are new users that follow an instruction
that assumes the initial branch name is 'master' would only need to
learn "here is how to change the branch name to match the tutorial
you are following in the repository you created for practice", and
"here is how you keep creating repositories with the first branch
with a name everybody hates" is unnecessary.
It also needs to be noted that the advise token to squelch the
message is the same advice.defaultBranchName as before, which is
also very much deliberate. The users who do have that configured
are those who _have_ been using Git since before 3.0, and they are
not the target audience for the new advice message. Reusing the
same advise token ensures that they do not have to turn the message
off.
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git range-diff" learned a way to limit the memory consumed by
O(N*N) cost matrix.
* pc/range-diff-memory-limit:
range-diff: add configurable memory limit for cost matrix
The clear_alloc_state() API function was not fully clearing the
structure for reuse, but since nobody reuses it, replace it with a
variant that frees the structure as well, making the callers simpler.
* ne/alloc-free-and-null:
alloc: fix dangling pointer in alloc_state cleanup
Adjust to the way newer versions of cURL selectivel enables tracing
options, so that our tests can continue to work.
* jk/curl-global-trace-components:
curl: add support for curl_global_trace() components
"git rev-parse --short" and friends failed to disambiguate two
objects with object names that share common prefix longer than 32
characters, which has been fixed.
* jc/longer-disambiguation-fix:
abbrev: allow extending beyond 32 chars to disambiguate
A corner case bug in "git log -L..." has been corrected.
* sg/line-log-boundary-fixes:
line-log: show all line ranges touched by the same diff range
line-log: fix assertion error
"git send-email" learned to drive "git imap-send" to store already
sent e-mails in an IMAP folder.
* ag/send-email-imap-sent:
send-email: enable copying emails to an IMAP folder without actually sending them
send-email: add ability to send a copy of sent emails to an IMAP folder
"core.commentChar=auto" that attempts to dynamically pick a
suitable comment character is non-workable, as it is too much
trouble to support for little benefit, and is marked as deprecated.
* pw/3.0-commentchar-auto-deprecation:
commit: print advice when core.commentString=auto
config: warn on core.commentString=auto
breaking-changes: deprecate support for core.commentString=auto
As the last commit deleted the only user of VERBATIM_MSG remove
it. This reverts remaining parts of commit f7d42ceec5 (rebase -i:
do leave commit message intact in fixup! chains, 2021-01-28) that
were not deleted by the last commit.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If the user uses a prepare-commit-msg hook to add comments to the
commit message template and sets commit.cleanup to remove them when the
commit is created then the comments will not be removed when rebase
commits the final command in a chain of "fixup" commands[1]. This
happens because f7d42ceec5 (rebase -i: do leave commit message intact
in fixup! chains, 2021-01-28) started passing the VERBATIM_MSG flag
when committing the final command in a chain of "fixup" commands. That
change was added in response to a bug report[2] where the commit
message was being cleaned up when it should not be. The cause of that
bug was that before f7d42ceec5 the sequencer passed CLEANUP_MSG
when committing the final fixup. That commit should have simply
removed the CLEANUP_MSG flag, not changed it to VERBATIM_MSG. Using
VERBATIM_MSG ignores the user's commit.cleanup config when committing
the final fixup which means it behaves differently to an ordinary
"pick" command which respects commit.cleanup.
Fix this by not setting an explicit cleanup flag when committing the
final fixup which matches the way "pick" commands behave. The test
added in f7d42ceec5 is replaced with one that checks that "fixup"
and "pick" commands do not clean up the message when commit.cleanup
is not set and do clean up the message when it is set.
[1] https://lore.kernel.org/git/CA+itcS3DxbgpFy2aPRvHQvTAYE=dU0kfeDdidVwWLU=rBAWR4w@mail.gmail.com
[2] https://lore.kernel.org/git/CANVGpwZGbzYLMeMze64e_OU9p3bjyEgzC5thmNBr6LttBt+YGw@mail.gmail.com
Reported-by: Simon Cheng <cyqsimon@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If I run
git send-email --compose --reply-to 'ME <my@address.net>' .....
and edit the intro message, then it will get two copies of the Reply-To
field. gmail.com rejects such messages.
This happens because send-email reads the edited message examining the
headers. For recognised headers the content is extracted to use in
constructing the final message and for possible inclusion in the patch
emails. Unrecognised headers are gathered (in @xh) to be passed through
uninterpreted.
Unfortunately "Reply-To" is not recognised in this process so it is
added to @xh as an uninterpreted header, but also generated from the
$reply_to variable in gen_header(), resulting in two copies
Add parsing to the loop in pre_process_file() to recognise a Reply-to
header and to store the result in $reply_to. This means that the
intro message will not get a second header and also means that
any changes made to the Reply-To header during editing will be
incorporated in the $reply_to variable and so included in all the
generated email messages.
Signed-off-by: NeilBrown <neil@brown.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Merges contributions made from three different addresses:
- win@wincent.com (old address, initial contributions in 2007–2009)
- greg@hurrell.net (personal address matching full name, so this one is
the "forever" address; contributions made starting in 2018)
- greg.hurrell@datadoghq.com (current work address, used for recent
contributions)
Signed-off-by: Greg Hurrell <greg.hurrell@datadoghq.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A broken or malicious "git fetch" can say that it has the same
object for many many times, and the upload-pack serving it can
exhaust memory storing them redundantly, which has been corrected.
* ps/upload-pack-oom-protection:
upload-pack: don't ACK non-commits repeatedly in protocol v2
t5530: modernize tests
Fixes multiple crashes around midx write-out codepaths.
* ds/midx-write-fixes:
midx-write: simplify error cases
midx-write: reenable signed comparison errors
midx-write: use uint32_t for preferred_pack_idx
midx-write: use cleanup when incremental midx fails
midx-write: put failing response value back
midx-write: only load initialized packs
"repo info" learns a short-hand option "-z" that is the same as
"--format=nul", and learns to report the objects format used in the
repository.
* lo/repo-info-step-2:
repo: add the field objects.format
repo: add the flag -z as an alias for --format=nul
The bulk-checkin code used to depend on a file-scope static
singleton variable, which has been updated to pass an instance
throughout the callchain.
* jt/de-global-bulk-checkin:
bulk-checkin: use repository variable from transaction
bulk-checkin: require transaction for index_blob_bulk_checkin()
bulk-checkin: remove global transaction state
bulk-checkin: introduce object database transaction structure
Instead of scanning for the remaining items to see if there are
still commits to be explored in the queue, use khash to remember
which items are still on the queue (an unacceptable alternative is
to reserve one object flag bits).
* rs/describe-with-lazy-queue-and-oidset:
describe: use oidset in finish_depth_computation()
Windows "real-time monitoring" interferes with the execution of
tests and affects negatively in both correctness and performance,
which has been disabled in Gitlab CI.
* ps/gitlab-ci-disable-windows-monitoring:
gitlab-ci: disable realtime monitoring to unbreak Windows jobs
"git refs exists" that works like "git show-ref --exists" has been
added.
* ms/refs-exists:
t: add test for git refs exists subcommand
t1422: refactor tests to be shareable
t1403: split 'show-ref --exists' tests into a separate file
builtin/refs: add 'exists' subcommand
Further code clean-up for multi-pack-index code paths.
* ps/object-store-midx-dedup-info:
midx: compute paths via their source
midx: stop duplicating info redundant with its owning source
midx: write multi-pack indices via their source
midx: load multi-pack indices via their source
midx: drop redundant `struct repository` parameter
odb: simplify calling `link_alt_odb_entry()`
odb: return newly created in-memory sources
odb: consistently use "dir" to refer to alternate's directory
odb: allow `odb_find_source()` to fail
odb: store locality in object database sources
