Plug the memory leaks from the trickiest API of all, the revision
walker.
* ab/plug-leak-in-revisions: (27 commits)
revisions API: add a TODO for diff_free(&revs->diffopt)
revisions API: have release_revisions() release "topo_walk_info"
revisions API: have release_revisions() release "date_mode"
revisions API: call diff_free(&revs->pruning) in revisions_release()
revisions API: release "reflog_info" in release revisions()
revisions API: clear "boundary_commits" in release_revisions()
revisions API: have release_revisions() release "prune_data"
revisions API: have release_revisions() release "grep_filter"
revisions API: have release_revisions() release "filter"
revisions API: have release_revisions() release "cmdline"
revisions API: have release_revisions() release "mailmap"
revisions API: have release_revisions() release "commits"
revisions API users: use release_revisions() for "prune_data" users
revisions API users: use release_revisions() with UNLEAK()
revisions API users: use release_revisions() in builtin/log.c
revisions API users: use release_revisions() in http-push.c
revisions API users: add "goto cleanup" for release_revisions()
stash: always have the owner of "stash_info" free it
revisions API users: use release_revisions() needing REV_INFO_INIT
revision.[ch]: document and move code declared around "init"
...
Add a missing reflog_walk_info_release() to "reflog-walk.c" and use it
in release_revisions().
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There were a few "git reflog exists" tests scattered over the test
suite, but let's consolidate the testing of the main functionality
into a new test file. This makes it easier to run just these tests
during development.
To do that amend and extend an existing test added in
afcb2e7a3b (git-reflog: add exists command, 2015-07-21). Let's use
"test_must_fail" instead of "!" (in case it segfaults), and test for
basic usage, an unknown option etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Carefully excluding t1309, which sees independent development elsewhere
at the time of writing, we transition above-mentioned tests to the
default branch name `main`. This trick was performed via
$ (cd t &&
sed -i -e 's/master/main/g' -e 's/MASTER/MAIN/g' \
-e 's/Master/Main/g' -e 's/naster/nain/g' -- t[01]*.sh &&
git checkout HEAD -- t1309\*)
Note that t5533 contains a variation of the name `master` (`naster`)
that we rename here, too.
This allows us to define `GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main`
for those tests.
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In addition to the manual adjustment to let the `linux-gcc` CI job run
the test suite with `master` and then with `main`, this patch makes sure
that GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME is set in all test scripts
that currently rely on the initial branch name being `master by default.
To determine which test scripts to mark up, the first step was to
force-set the default branch name to `master` in
- all test scripts that contain the keyword `master`,
- t4211, which expects `t/t4211/history.export` with a hard-coded ref to
initialize the default branch,
- t5560 because it sources `t/t556x_common` which uses `master`,
- t8002 and t8012 because both source `t/annotate-tests.sh` which also
uses `master`)
This trick was performed by this command:
$ sed -i '/^ *\. \.\/\(test-lib\|lib-\(bash\|cvs\|git-svn\)\|gitweb-lib\)\.sh$/i\
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\
' $(git grep -l master t/t[0-9]*.sh) \
t/t4211*.sh t/t5560*.sh t/t8002*.sh t/t8012*.sh
After that, careful, manual inspection revealed that some of the test
scripts containing the needle `master` do not actually rely on a
specific default branch name: either they mention `master` only in a
comment, or they initialize that branch specificially, or they do not
actually refer to the current default branch. Therefore, the
aforementioned modification was undone in those test scripts thusly:
$ git checkout HEAD -- \
t/t0027-auto-crlf.sh t/t0060-path-utils.sh \
t/t1011-read-tree-sparse-checkout.sh \
t/t1305-config-include.sh t/t1309-early-config.sh \
t/t1402-check-ref-format.sh t/t1450-fsck.sh \
t/t2024-checkout-dwim.sh \
t/t2106-update-index-assume-unchanged.sh \
t/t3040-subprojects-basic.sh t/t3301-notes.sh \
t/t3308-notes-merge.sh t/t3423-rebase-reword.sh \
t/t3436-rebase-more-options.sh \
t/t4015-diff-whitespace.sh t/t4257-am-interactive.sh \
t/t5323-pack-redundant.sh t/t5401-update-hooks.sh \
t/t5511-refspec.sh t/t5526-fetch-submodules.sh \
t/t5529-push-errors.sh t/t5530-upload-pack-error.sh \
t/t5548-push-porcelain.sh \
t/t5552-skipping-fetch-negotiator.sh \
t/t5572-pull-submodule.sh t/t5608-clone-2gb.sh \
t/t5614-clone-submodules-shallow.sh \
t/t7508-status.sh t/t7606-merge-custom.sh \
t/t9302-fast-import-unpack-limit.sh
We excluded one set of test scripts in these commands, though: the range
of `git p4` tests. The reason? `git p4` stores the (foreign) remote
branch in the branch called `p4/master`, which is obviously not the
default branch. Manual analysis revealed that only five of these tests
actually require a specific default branch name to pass; They were
modified thusly:
$ sed -i '/^ *\. \.\/lib-git-p4\.sh$/i\
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master\
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\
' t/t980[0167]*.sh t/t9811*.sh
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Test fixes.
* sg/test-must-be-empty:
tests: use 'test_must_be_empty' instead of 'test_cmp <empty> <out>'
tests: use 'test_must_be_empty' instead of 'test_cmp /dev/null <out>'
tests: use 'test_must_be_empty' instead of 'test ! -s'
tests: use 'test_must_be_empty' instead of '! test -s'
Using 'test_must_be_empty' is shorter and more idiomatic than
>empty &&
test_cmp empty out
as it saves the creation of an empty file. Furthermore, sometimes the
expected empty file doesn't have such a descriptive name like 'empty',
and its creation is far away from the place where it's finally used
for comparison (e.g. in 't7600-merge.sh', where two expected empty
files are created in the 'setup' test, but are used only about 500
lines later).
These cases were found by instrumenting 'test_cmp' to error out the
test script when it's used to compare empty files, and then converted
manually.
Note that even after this patch there still remain a lot of cases
where we use 'test_cmp' to check empty files:
- Sometimes the expected output is not hard-coded in the test, but
'test_cmp' is used to ensure that two similar git commands produce
the same output, and that output happens to be empty, e.g. the
test 'submodule update --merge - ignores --merge for new
submodules' in 't7406-submodule-update.sh'.
- Repetitive common tasks, including preparing the expected results
and running 'test_cmp', are often extracted into a helper
function, and some of this helper's callsites expect no output.
- For the same reason as above, the whole 'test_expect_success'
block is within a helper function, e.g. in 't3070-wildmatch.sh'.
- Or 'test_cmp' is invoked in a loop, e.g. the test 'cvs update
(-p)' in 't9400-git-cvsserver-server.sh'.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Adjust the test so that it uses a variable consisting of the current
HEAD instead of a hard-coded hash.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since its inception, the general strategy of the reflog-walk
code has been to start with the tip commit for the ref, and
as we traverse replace each commit's parent pointers with
fake parents pointing to the previous reflog entry.
This lets us traverse the reflog as if it were a real
history, but it has some user-visible oddities. Namely:
1. The fake parents are used for commit selection and
display. So for example, "--merges" or "--no-merges"
are not useful, because the history appears as a linear
string of commits. Likewise, pathspec limiting is based
on the diff between adjacent entries, not the changes
actually introduced by a commit.
These are often the same (e.g., because the entry was
just running "git commit" and the adjacent entry _is_
the true parent), but it may not be in several common
cases. For instance, using "git reset" to jump around
history, or "git checkout" to move HEAD.
2. We reverse-map each commit back to its reflog. So when
it comes time to show commit X, we say "a-ha, we added
X because it was at the tip of the 'foo' reflog, so
let's show the foo reflog". But this leads to nonsense
results when you ask to traverse multiple reflogs: if
two reflogs have the same tip commit, we only map back
to one of them. Instead, we should show both.
3. If the tip of the reflog and the ref tip disagree on
the current value, we show the ref tip but give no
indication of the value in the reflog. This situation
isn't supposed to happen (since any ref update should
touch the reflog). But if it does, given that the
requested operation is to show the reflog, it makes
sense to prefer that.
This commit adds a new script with several expect_failure
tests to demonstrate the problems. This could be part of
the existing t1411, but it's a bit easier to start from a
fresh state, where we know exactly what will be in the log.
Since the new multiple-reflog tests are checking the actual
output, we can drop the "make sure we don't segfault" tests
from t1411, which are a strict subset of what we're doing
here.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The add_reflog_for_walk() function keeps a cache mapping
refnames to their reflog contents. We use a cached reflog
entry if available, and otherwise allocate and store a new
one.
Since 5026b47175 (add_reflog_for_walk: avoid memory leak,
2017-05-04), when we hit an error parsing a date-based
reflog spec, we free the reflog memory but leave the cache
entry pointing to the now-freed memory.
We can fix this by just leaving the memory intact once it
has made it into the cache. This may leave an unused entry
in the cache, but that's OK. And it means we also catch a
similar situation: we may not have allocated at all in this
invocation, but simply be pointing to a cached entry from a
previous invocation (which is relying on that entry being
present).
The new test in t1411 exercises this case and fails when run
with --valgrind or ASan.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
As part of the add_reflog_to_walk() function, we keep a
string_list mapping refnames to their reflog contents. This
serves as a cache so that accessing the same reflog twice
requires only a single copy of the log in memory.
The string_list is initialized via xcalloc, meaning its
strdup_strings field is set to 0. But after inserting a
string into the list, we unconditionally call free() on the
string, leaving the list pointing to freed memory. If
another reflog is added (e.g., "git log -g HEAD HEAD"), then
the second one may have unpredictable results.
The extra free was added by 5026b47175 (add_reflog_for_walk:
avoid memory leak, 2017-05-04). Though if you look
carefully, you can see that the code was buggy even before
then. If we tried to read the reflogs by time but came up
with no entries, we exited with an error, freeing the string
in that code path. So the bug was harder to trigger, but
still there.
We can fix it by just asking the string list to make a copy
of the string. Technically we could fix the problem by not
calling free() on our string (and just handing over
ownership to the string list), but there are enough
conditionals that it's quite hard to figure out which code
paths need the free and which do not. Simpler is better
here.
The new test reliably shows the problem when run with
--valgrind or ASAN.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Tests that assume how reflogs are represented on the filesystem too
much have been corrected.
* dt/reflog-tests:
tests: remove some direct access to .git/logs
t/t7509: remove unnecessary manipulation of reflog
Alternate refs backends might store reflogs somewhere other than
.git/logs. Change most test code that directly accesses .git/logs to
instead use git reflog commands.
There are still a few tests which need direct access to reflogs: to
check reflog permissions, to manually create reflogs from scratch, to
save/restore reflogs, to check the format of raw reflog data, and to
remove not just reflog contents, but the reflogs themselves. All cases
which don't need direct access have been modified.
Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is necessary because alternate ref backends might store reflogs
somewhere other than .git/logs. Code that now directly manipulates
.git/logs should instead go through git-reflog.
Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The reflog walking logic (git log -g) replaces the true parent list
with the preceding commit in the reflog. This results in bogus commit
diffs when combined with options such as -p; the diff is against the
reflog predecessor, not the parent of the commit.
Save the true parents on the side, extending the functions from the
previous commit. The diff logic picks them up and uses them to show
the correct diffs.
We do have to be somewhat careful about repeated calling of
save_parents(), since the reflog may list a commit more than once. We
now store (commit_list*)-1 to distinguish the "not saved yet" and
"root commit" cases. This lets us preserve an empty parent list even
if save_parents() is repeatedly called.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduction of opt->date_mode_explicit was a step in the right direction,
but lost that crucial bit at the very end of the callchain, and the callee
could not tell an explicitly specified "I want *date* but in default format"
from the built-in default value passed when there was no --date specified.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we are showing reflog selectors during a walk, we infer
from context whether the user wanted to see the index in
each selector, or the reflog date. The current rules are:
1. if the user asked for an explicit date format in the
output, show the date
2. if the user asked for ref@{now}, show the date
3. if neither is true, show the index
However, if we see "ref@{0}", that should be a strong clue
that the user wants to see the counted version. In fact, it
should be much stronger than the date format in (1). The
user may have been setting the date format to use in another
part of the output (e.g., in --format="%gd (%ad)", they may
have wanted to influence the author date).
This patch flips the rules to:
1. if the user asked for ref@{0}, always show the index
2. if the user asked for ref@{now}, always show the date
3. otherwise, we have just "ref"; show them counted by
default, but respect the presence of "--date" as a clue
that the user wanted them date-based
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we show a reflog selector (e.g., via "git log -g"), we
perform some DWIM magic: while we normally show the entry's
index (e.g., HEAD@{1}), if the user has given us a date
with "--date", then we show a date-based select (e.g.,
HEAD@{yesterday}).
However, we don't want to trigger this magic if the
alternate date format we got was from the "log.date"
configuration; that is not sufficiently strong context for
us to invoke this particular magic. To fix this, commit
f4ea32f (improve reflog date/number heuristic, 2009-09-24)
introduced a "date_mode_explicit" flag in rev_info. This
flag is set only when we see a "--date" option on the
command line, and we a vanilla date to the reflog code if
the date was not explicit.
Later, commit 8f8f547 (Introduce new pretty formats %g[sdD]
for reflog information, 2009-10-19) added another way to
show selectors, and it did not respect the date_mode_explicit
flag from f4ea32f.
This patch propagates the date_mode_explicit flag to the
pretty-print code, which can then use it to pass the
appropriate date field to the reflog code. This brings the
behavior of "%gd" in line with the other formats, and means
that its output is independent of any user configuration.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We already check that @{now} and "--date" cause the
displayed selector to use the date for both the multiline
and oneline formats. However, we miss several cases:
1. The --format=%gd selector is not tested at all.
2. We do not check how the log.date config interacts with the
"--date" magic (according to f4ea32f, it should not
impact the output).
Doing so reveals that the combination of both (log.date
combined with the %gd format) does not behave as expected.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently, "git reflog" overrides some command line options such as
"--format".
Fix this by using the new 2-phase version of cmd_log_init().
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git reflog --format=short" does not work because "reflog" overrides the
format option. This is documented in code. Document this by a test
(known failure) also.
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If a revision is specified, it happens not to have any commits, don't
use the default revision. By doing so, surprising and undesired
behavior can happen, such as showing the reflog for HEAD when a branch
was specified.
[jc: squashed a test from René]
Signed-off-by: Dave Olszewski <cxreg@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The multiline reflog format (e.g., as shown by "git log -g")
will show HEAD@{<date>} rather than HEAD@{<count>} in two
situations:
1. If the user gave branch@{<date>} syntax to specify the
reflog
2. If the user gave a --date=<format> specifier
It uses the "normal" date format in case 1, and the
user-specified format in case 2.
The oneline reflog format (e.g., "git reflog show" or "git
log -g --oneline") will show the date in the same two
circumstances. However, it _always_ shows the date as a
relative date, and it always ignores the timezone.
In case 2, it seems ridiculous to trigger the date but use a
format totally different from what the user requested.
For case 1, it is arguable that the user might want to see
the relative date by default; however, the multiline version
shows the normal format.
This patch does three things:
- refactors the "relative_date" parameter to
show_reflog_message to be an actual date_mode enum,
since this is how it is used (it is passed to show_date)
- uses the passed date_mode parameter in the oneline
format (making it consistent with the multiline format)
- does not ignore the timezone parameter in oneline mode
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
