Many codepaths did not diagnose write failures correctly when disks
go full, due to their misuse of write_in_full() helper function,
which have been corrected.
* jk/write-in-full-fix:
read_pack_header: handle signed/unsigned comparison in read result
config: flip return value of store_write_*()
notes-merge: use ssize_t for write_in_full() return value
pkt-line: check write_in_full() errors against "< 0"
convert less-trivial versions of "write_in_full() != len"
avoid "write_in_full(fd, buf, len) != len" pattern
get-tar-commit-id: check write_in_full() return against 0
config: avoid "write_in_full(fd, buf, len) < len" pattern
Doc update.
* kd/doc-for-each-ref:
doc/for-each-ref: explicitly specify option names
doc/for-each-ref: consistently use '=' to between argument names and values
Finishing touches to a topic already in 'master'.
* cc/subprocess-handshake-missing-capabilities:
subprocess: loudly die when subprocess asks for an unsupported capability
A hotfix to a topic already in 'master'.
* kw/write-index-reduce-alloc:
read-cache: fix index corruption with index v4
Add t/helper/test-write-cache to .gitignore
A handful of tests to demonstrates a recursive implementation of
"name-rev" hurts.
* mg/name-rev-tests-with-short-stack:
t6120: test describe and name-rev with deep repos
t6120: clean up state after breaking repo
t6120: test name-rev --all and --stdin
t7004: move limited stack prereq to test-lib
Currently, when parse_options() produces a help message it always emits
a blank line after the usage text to separate it from the options text.
If the option spec does not define any switches, or only defines hidden
switches that will not be displayed, then the help text will end up with
two trailing blank lines instead of one. Let's defer emitting the blank
line between the usage text and the options text until it is clear that
the options section will not be empty.
Fixes t1502.5, t1502.6.
Signed-off-by: Brandon Casey <drafnel@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When commit 54e6dc7 added translation support to parse-options, an
fprintf was mistakenly replaced by a call to putchar(). Let's use fputc
instead.
Fixes t0040.11, t0040.12, t0040.33, and t1502.8.
Signed-off-by: Brandon Casey <drafnel@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When the option spec contains no switches or only hidden switches,
parse_options will emit an extra blank line at the end of help output so
that the help text will end in two blank lines instead of one.
When parse_options produces internal help output after an error has
occurred it will emit blank lines within the usage string to stdout
instead of stderr.
Update t/helper/test-parse-options.c to have a description body in the
usage string to exercise this second bug and mark tests as failing in
t0040.
Add tests to t1502 to demonstrate both of these problems.
Signed-off-by: Brandon Casey <drafnel@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The description of `-Xours` merge option has a parenthetical note
that tells the readers that it is very different from `-s ours`,
which is correct, but the description of `-Xtheirs` that follows it
carelessly says "this is the opposite of `ours`", giving a false
impression that the readers also need to be warned that it is very
different from `-s theirs`, which in reality does not even exist.
Clarify it a bit to avoid misleading readers.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
With more commands (that potentially change a submodule) paying attention
to submodules as well as the recent discussion[1] on
submodule.<name>.update, let's spell out that submodule.<name>.update
is strictly to be used for configuring the "submodule update" command
and not to be obeyed by other commands.
These other commands usually have a strict meaning of what they should
do (i.e. checkout, reset, rebase, merge) as well as have their name
overlapping with the modes possible for submodule.<name>.update.
[1] https://public-inbox.org/git/4283F0B0-BC1C-4ED1-8126-7E512D84484B@gmail.com/
submodule.<name>.update was set to "none", triggering unexpected
behavior as the submodule was thought to never be touched.
However a newer version of Git taught 'git pull --rebase' to also
populate and rebase submodules if they were active.
The newer options such as submodule.active and command specific
flags would not have triggered unexpected behavior.
Reported-by: Lars Schneider <larsxschneider@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Decode =XY in quoted-printable segments only if X and Y are hexadecimal
digits, otherwise just copy them. That's at least better than
interpreting negative results from hexval() as a character.
Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This allows us to get rid of some write-only variables, among them seven
SHA1 buffers.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This allows us to get rid of two write-only variables, one of them
being a SHA1 buffer.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Allow callers of refs_resolve_ref_unsafe() to pass NULL if they don't
need the resolved hash value. We already allow the same for the flags
parameter. This new leniency is inherited by the various wrappers like
resolve_ref_unsafe().
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Current HTML header regexp doesn't match headers without attributes.
So it fails to match <h1>...</h1>, while <h1 class="smth">...</h1> matches.
Make attributes optional to fix this. The regexp is still far from
perfect, but now it at least handles the common case.
Signed-off-by: Ilya Kantor <iliakan@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The git-read-tree manpage has a table that is meant to
be shown with its spacing exactly as it is in the source. We
mark it as a "literal paragraph" by indenting each line by
at least one space. This renders OK with asciidoc for both
the HTML and manpage versions.
But there are two problems when we render it with
asciidoctor.
The first is that some lines mix tabs and spaces. Even if
asciidoctor is correctly configured for 8-space tabs, it
seems to handle this case differently, soaking up some of
the initial literal-paragraph spaces and mis-aligning the
table text.
The second problem is that the table uses blank lines to
group rows. But as blank lines separate paragraphs in
asciidoc, this actually means that each chunk of the table
is rendered in its own pre-formatted <div> block. This
happens even with vanilla asciidoc, but there's no visible
result because the literal paragraphs aren't styled in any
special way. But with asciidoctor (or at least the styles
used on git-scm.com), literal paragraphs are styled with a
different background. This breaks the table into a visually
distracting sequence of chunks.
We can fix both by adding a literal-paragraph block
delimiter. That turns the whole table into a single block
(for both implementations) and causes asciidoctor to render
the indentation as it is in the source.
Reported-at: https://github.com/git/git-scm.com/issues/1023
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of setting the fields of rev->pending to 0/NULL, thereby leaking
memory, call `object_array_clear(&rev->pending)`.
In pack-bitmap.c, we make copies of those fields as `pending_nr` and
`pending_e`. We never update the aliases and the original fields never
change, so the aliases are not really needed and just make it harder
than necessary to understand the code. While we're here, remove the
aliases to make the code easier to follow.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a couple of places, we pop objects off an object array `foo` by
decreasing `foo.nr`. We access `foo.nr` in many places, but most if not
all other times we do so read-only, e.g., as we iterate over the array.
But when we change `foo.nr` behind the array's back, it feels a bit
nasty and looks like it might leak memory.
Leaks happen if the popped element has an allocated `name` or `path`.
At the moment, that is not the case. Still, 1) the object array might
gain more fields that want to be freed, 2) a code path where we pop
might start using names or paths, 3) one of these code paths might be
copied to somewhere where we do, and 4) using a dedicated function for
popping is conceptually cleaner.
Introduce and use `object_array_pop()` instead. Release memory in the
new function. Document that popping an object leaves the associated
elements in limbo.
The converted places were identified by grepping for "\.nr\>" and
looking for "--".
Make the new function return NULL on an empty array. This is consistent
with `pop_commit()` and allows the following:
while ((o = object_array_pop(&foo)) != NULL) {
// do something
}
But as noted above, we don't need to go out of our way to avoid reading
`foo.nr`. This is probably more readable:
while (foo.nr) {
... o = object_array_pop(&foo);
// do something
}
The name of `object_array_pop()` does not quite align with
`add_object_array()`. That is unfortunate. On the other hand, it matches
`object_array_clear()`. Arguably it's `add_...` that is the odd one out,
since it reads like it's used to "add" an "object array". For that
reason, side with `object_array_clear()`.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of freeing `foo.objects` for an object array `foo` (sometimes
conditionally), call `object_array_clear(&foo)`. This means we don't
poke as much into the implementation, which is already a good thing, but
also that we release the individual entries as well, thereby fixing at
least one memory-leak (in diff-lib.c).
If someone is holding on to a pointer to an element's `name` or `path`,
that is now a dangling pointer, i.e., we'd be turning an unpleasant
situation into an outright bug. To the best of my understanding no such
long-term pointers are being taken.
The way we handle `study` in builting/reflog.c still looks like it might
leak. That will be addressed in the next commit.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Setting `leak_pending = 1` tells `prepare_revision_walk()` not to
release the `pending` array, and makes that the caller's responsibility.
See 4a43d374f (revision: add leak_pending flag, 2011-10-01) and
353f5657a (bisect: use leak_pending flag, 2011-10-01).
Commit 1da1e07c8 (clean up name allocation in prepare_revision_walk,
2014-10-15) fixed a memory leak in `prepare_revision_walk()` by
switching from `free()` to `object_array_clear()`. However, where we use
the `leak_pending`-mechanism, we're still only calling `free()`.
Use `object_array_clear()` instead. Copy some helpful comments from
353f5657a to the other callers that we update to clarify the memory
responsibilities, and to highlight that the commits are not affected
when we clear the array -- it is indeed correct to both tidy up the
commit flags and clear the object array.
Document `leak_pending` in revision.h to help future users get this
right.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We don't free the temporary scratch space we use with
`remove_redundant()`. Free it similar to how we do it in
`get_merge_bases_many_0()`.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Release `pathspec` and the string list `partial`.
When we clear the string list, make sure we do not free the `util`
pointers. That would result in double-freeing, since we set them up as
`item->util = item` in `list_paths()`.
Initialize the string list early, so that we can always release it. That
introduces some unnecessary overhead in various code paths, but means
there is one and only one way out of the function. If we ever accumulate
more things we need to free, it should be straightforward to do so.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When creating a new branch B by copying the branch A that happens to
be the current branch, it also updates HEAD to point at the new
branch. It probably was made this way because "git branch -c A B"
piggybacked its implementation on "git branch -m A B",
This does not match the usual expectation. If I were sitting on a
blue chair, and somebody comes and repaints it to red, I would
accept ending up sitting on a chair that is now red (I am also OK to
stand, instead, as there no longer is my favourite blue chair). But
if somebody creates a new red chair, modelling it after the blue
chair I am sitting on, I do not expect to be booted off of the blue
chair and ending up on sitting on the new red one.
Let's fix this before it hits 'next'. Those who want to create a
new branch and switch to it can do "git checkout B" after doing a
"git branch -c B", and if that operation is so useful and deserves a
short-hand way to do so, perhaps extend "git checkout -b B" to copy
configurations while creating the new branch B.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This allows us to recreate even historical tags which would now be consider
invalid, such as v2.6.12-rc2..v2.6.13-rc3 in the Linux kernel source tree which
lack the `tagger` header.
$ git rev-parse v2.6.12-rc2
9e734775f7c22d2f89943ad6c745571f1930105f
$ git cat-file tag v2.6.12-rc2 | git mktag
error: char76: could not find "tagger "
fatal: invalid tag signature file
$ git cat-file tag v2.6.12-rc2 | git hash-object -t tag -w --stdin
9e734775f7c22d2f89943ad6c745571f1930105f
Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
With "--state-branch=<branchname>" option, the mapping from old object names
and filtered ones in ./map/ directory is stashed away in the object database,
and the one from the previous run is read to populate the ./map/ directory,
allowing for incremental updates of large trees.
Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
These are modified by set_ident() but a subsequent patch would like to operate
on their original values.
Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This is pure code motion to enable a subsequent patch to add code which needs
to happen with the reset $GIT_* but before the temporary directory has been
cleaned up.
Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Gcc 7 adds -Wimplicit-fallthrough, which can warn when a
switch case falls through to the next case. The general idea
is that the compiler can't tell if this was intentional or
not, so you should annotate any intentional fall-throughs as
such, leaving it to complain about any unannotated ones.
There's a GNU __attribute__ which can be used for
annotation, but of course we'd have to #ifdef it away on
non-gcc compilers. Gcc will also recognize
specially-formatted comments, which matches our current
practice. Let's extend that practice to all of the
unannotated sites (which I did look over and verify that
they were behaving as intended).
Ideally in each case we'd actually give some reasons in the
comment about why we're falling through, or what we're
falling through to. And gcc does support that with
-Wimplicit-fallthrough=2, which relaxes the comment pattern
matching to anything that contains "fallthrough" (or a
variety of spelling variants). However, this isn't the
default for -Wimplicit-fallthrough, nor for -Wextra. In the
name of simplicity, it's probably better for us to support
the default level, which requires "fallthrough" to be the
only thing in the comment (modulo some window dressing like
"else" and some punctuation; see the gcc manual for the
complete set of patterns).
This patch suppresses all warnings due to
-Wimplicit-fallthrough. We might eventually want to add that
to the DEVELOPER Makefile knob, but we should probably wait
until gcc 7 is more widely adopted (since earlier versions
will complain about the unknown warning type).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Our trace handler is called by curl with a curl_infotype
variable to interpret its data field. For most types we
print the data and then break out of the switch. But for
CURLINFO_TEXT, we print data and then fall through to the
"default" case, which does the exact same thing (nothing!)
that breaking out of the switch would.
This is probably a leftover from an early iteration of the
patch where the code after the switch _did_ do something
interesting that was unique to the non-text case arms.
But in its current form, this fallthrough is merely
confusing (and causes gcc's -Wimplicit-fallthrough to
complain).
Let's make CURLINFO_TEXT like the other case arms, and push
the default arm to the end where it's more obviously a
catch-all.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The handle_command() function matches an incoming command
string with a sequence of starts_with() checks. But it also
surrounds these with a switch on the first character of the
command, which lets us jump to the right block of
starts_with() without going linearly through the list.
However, each case arm of the switch falls through to the
one below it. This is pointless (we know that a command
starting with 'b' does not need to check any of the commands
in the 'c' block), and it makes gcc's -Wimplicit-fallthrough
complain.
We could solve this by adding a break at the end of each
block. However, this optimization isn't helping anything.
Even if it does make matching faster (which is debatable),
this is code that is run only in the test suite, and each
run receives at most two of these "commands". We should
favor simplicity and readability over micro-optimizing.
Instead, let's drop the switch statement completely and
replace it with an if/else cascade.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commit dc944b65f1 (get_sha1_with_context: dynamically
allocate oc->path, 2017-05-19) changed the rules that
callers must follow for seeing if we parsed a path in the
object name. The rules switched from "check if the oc.path
buffer is empty" to "check if the oc.path pointer is NULL".
But that commit forgot to update some sites in
cat_one_file(), meaning we might dereference a NULL pointer.
You can see this by making a path-aware request like
--textconv without specifying --path, and giving an object
name that doesn't have a path in it. Like:
git cat-file --textconv HEAD
which will reliably segfault.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
