summaryrefslogtreecommitdiff
path: root/fast-import.c
AgeCommit message (Collapse)AuthorFilesLines
2017-05-08pack: convert struct pack_idx_entry to struct object_idLibravatar brian m. carlson1-15/+15
Convert struct pack_idx_entry to use struct object_id by changing the definition and applying the following semantic patch, plus the standard object_id transforms: @@ struct pack_idx_entry E1; @@ - E1.sha1 + E1.oid.hash @@ struct pack_idx_entry *E1; @@ - E1->sha1 + E1->oid.hash Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-08Convert lookup_commit* to struct object_idLibravatar brian m. carlson1-2/+2
Convert lookup_commit, lookup_commit_or_die, lookup_commit_reference, and lookup_commit_reference_gently to take struct object_id arguments. Introduce a temporary in parse_object buffer in order to convert this function. This is required since in order to convert parse_object and parse_object_buffer, lookup_commit_reference_gently and lookup_commit_or_die would need to be converted. Not introducing a temporary would therefore require that lookup_commit_or_die take a struct object_id *, but lookup_commit would take unsigned char *, leaving a confusing and hard-to-use interface. parse_object_buffer will lose this temporary in a later patch. This commit was created with manual changes to commit.c, commit.h, and object.c, plus the following semantic patch: @@ expression E1, E2; @@ - lookup_commit_reference_gently(E1.hash, E2) + lookup_commit_reference_gently(&E1, E2) @@ expression E1, E2; @@ - lookup_commit_reference_gently(E1->hash, E2) + lookup_commit_reference_gently(E1, E2) @@ expression E1; @@ - lookup_commit_reference(E1.hash) + lookup_commit_reference(&E1) @@ expression E1; @@ - lookup_commit_reference(E1->hash) + lookup_commit_reference(E1) @@ expression E1; @@ - lookup_commit(E1.hash) + lookup_commit(&E1) @@ expression E1; @@ - lookup_commit(E1->hash) + lookup_commit(E1) @@ expression E1, E2; @@ - lookup_commit_or_die(E1.hash, E2) + lookup_commit_or_die(&E1, E2) @@ expression E1, E2; @@ - lookup_commit_or_die(E1->hash, E2) + lookup_commit_or_die(E1, E2) Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-08fast-import: convert to struct object_idLibravatar brian m. carlson1-158/+159
Convert the remaining parts of fast-import.c to use struct object_id. Convert several instances of get_sha1_hex to parse_oid_hex to avoid needing to specify constants. Convert other hardcoded values to named constants. Finally, use the is_empty_tree_oid function instead of a direct comparison against a fixed string. Note that the odd computation with GIT_MAX_HEXSZ is due to the insertion of a slash between every two hex digits in the path, plus one for the terminating NUL. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-05-02fast-import: convert internal structs to struct object_idLibravatar brian m. carlson1-86/+96
Convert struct tree_entry_ms, struct branch, struct tag, and struct hash_list to use struct object_id by changing the definition and applying the following semantic patch, plus the standard object_id transforms: @@ struct tree_entry_ms E1; @@ - E1.sha1 + E1.oid.hash @@ struct tree_entry_ms *E1; @@ - E1->sha1 + E1->oid.hash @@ struct branch E1; @@ - E1.sha1 + E1.oid.hash @@ struct branch *E1; @@ - E1->sha1 + E1->oid.hash @@ struct tag E1; @@ - E1.sha1 + E1.oid.hash @@ struct tag *E1; @@ - E1->sha1 + E1->oid.hash @@ struct hash_list E1; @@ - E1.sha1 + E1.oid.hash @@ struct hash_list *E1; @@ - E1->sha1 + E1->oid.hash Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-04-26Merge branch 'jk/war-on-git-path'Libravatar Junio C Hamano1-1/+1
While handy, "git_path()" is a dangerous function to use as a callsite that uses it safely one day can be broken by changes to other code that calls it. Reduction of its use continues. * jk/war-on-git-path: am: drop "dir" parameter from am_state_init replace strbuf_addstr(git_path()) with git_path_buf() replace xstrdup(git_path(...)) with git_pathdup(...) use git_path_* helper functions branch: add edit_description() helper bisect: add git_path_bisect_terms helper
2017-04-20replace xstrdup(git_path(...)) with git_pathdup(...)Libravatar Jeff King1-1/+1
It's more efficient to use git_pathdup(), as it skips an extra copy of the path. And by removing some calls to git_path(), it makes it easier to audit for dangerous uses. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-03-28odb_mkstemp: write filename into strbufLibravatar Jeff King1-4/+5
The odb_mkstemp() function expects the caller to provide a fixed buffer to write the resulting tempfile name into. But it creates the template using snprintf without checking the return value. This means we could silently truncate the filename. In practice, it's unlikely that the truncation would end in the template-pattern that mkstemp needs to open the file. So we'd probably end up failing either way, unless the path was specially crafted. The simplest fix would be to notice the truncation and die. However, we can observe that most callers immediately xstrdup() the result anyway. So instead, let's switch to using a strbuf, which is easier for them (and isn't a big deal for the other 2 callers, who can just strbuf_release when they're done with it). Note that many of the callers used static buffers, but this was purely to avoid putting a large buffer on the stack. We never passed the static buffers out of the function, so there's no complicated memory handling we need to change. Signed-off-by: Jeff King <peff@peff.net>
2017-03-28Merge branch 'jk/fast-import-cleanup'Libravatar Junio C Hamano1-9/+7
Code clean-up. * jk/fast-import-cleanup: pack.h: define largest possible encoded object size encode_in_pack_object_header: respect output buffer length fast-import: use xsnprintf for formatting headers fast-import: use xsnprintf for writing sha1s
2017-03-24encode_in_pack_object_header: respect output buffer lengthLibravatar Jeff King1-5/+5
The encode_in_pack_object_header() writes a variable-length header to an output buffer, but it doesn't actually know long the buffer is. At first glance, this looks like it might be possible to overflow. In practice, this is probably impossible. The smallest buffer we use is 10 bytes, which would hold the header for an object up to 2^67 bytes. Obviously we're not likely to see such an object, but we might worry that an object could lie about its size (causing us to overflow before we realize it does not actually have that many bytes). But the argument is passed as a uintmax_t. Even on systems that have __int128 available, uintmax_t is typically restricted to 64-bit by the ABI. So it's unlikely that a system exists where this could be exploited. Still, it's easy enough to use a normal out/len pair and make sure we don't write too far. That protects the hypothetical 128-bit system, makes it harder for callers to accidentally specify a too-small buffer, and makes the resulting code easier to audit. Note that the one caller in fast-import tried to catch such a case, but did so _after_ the call (at which point we'd have already overflowed!). This check can now go away. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-03-24fast-import: use xsnprintf for formatting headersLibravatar Jeff King1-3/+1
The stream_blob() function checks the return value of snprintf and dies. This is more simply done with xsnprintf (and matches the similar call in store_object). The message the user would get is less specific, but since the point is that this _shouldn't_ ever happen, that's OK. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-03-24fast-import: use xsnprintf for writing sha1sLibravatar Jeff King1-1/+1
When we have to write a sha1 with a newline, we do so by copying both into a single buffer, so that we can issue a single write() call. We use snprintf but don't bother to check the output, since we know it will fit. However, we should use xsnprintf() in such a case so that we're notified if our assumption turns out to be wrong (and to make it easier to audit for unchecked snprintf calls). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-03-21Merge branch 'jk/pack-name-cleanups'Libravatar Junio C Hamano1-13/+13
Code clean-up. * jk/pack-name-cleanups: index-pack: make pointer-alias fallbacks safer replace snprintf with odb_pack_name() odb_pack_keep(): stop generating keepfile name sha1_file.c: make pack-name helper globally accessible move odb_* declarations out of git-compat-util.h
2017-03-16replace snprintf with odb_pack_name()Libravatar Jeff King1-15/+13
In several places we write the name of the pack filename into a fixed-size buffer using snprintf(), but do not check the return value. As a result, a very long object directory could cause us to quietly truncate the pack filename (potentially leading to a corrupted repository, as a newly written packfile could be missing its .pack extension). We can use odb_pack_name() to do this with a strbuf (and shorten the code, as well). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-03-16odb_pack_keep(): stop generating keepfile nameLibravatar Jeff King1-1/+3
The odb_pack_keep() function generates the name of a .keep file and opens it. This has two problems: 1. It requires a fixed-size buffer to create the filename and doesn't notice when the result is truncated. 2. Of the two callers, one sometimes wants to open a filename it already has, which makes things awkward (it has to do so manually, and skips the leading-directory creation). Instead, let's have odb_pack_keep() just open the file. Generating the name isn't hard, and a future patch will switch callers over to odb_pack_name() anyway. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-02-20delete_ref: accept a reflog message argumentLibravatar Kyle Meyer1-1/+1
When the current branch is renamed with 'git branch -m/-M' or deleted with 'git update-ref -m<msg> -d', the event is recorded in HEAD's log with an empty message. In preparation for adding a more meaningful message to HEAD's log in these cases, update delete_ref() to take a message argument and pass it along to ref_transaction_delete(). Modify all callers to pass NULL for the new message argument; no change in behavior is intended. Note that this is relevant for HEAD's log but not for the deleted ref's log, which is currently deleted along with the ref. Even if it were not, an entry for the deletion wouldn't be present in the deleted ref's log. files_transaction_commit() writes to the log if REF_NEEDS_COMMIT or REF_LOG_ONLY are set, but lock_ref_for_update() doesn't set REF_NEEDS_COMMIT for the deleted ref because REF_DELETING is set. In contrast, the update for HEAD has REF_LOG_ONLY set by split_head_update(), resulting in the deletion being logged. Signed-off-by: Kyle Meyer <kyle@kyleam.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2017-01-10Merge branch 'mh/fast-import-notes-fix-new'Libravatar Junio C Hamano1-3/+5
"git fast-import" sometimes mishandled while rebalancing notes tree, which has been fixed. * mh/fast-import-notes-fix-new: fast-import: properly fanout notes when tree is imported
2016-12-20fast-import: properly fanout notes when tree is importedLibravatar Mike Hommey1-3/+5
In typical uses of fast-import, trees are inherited from a parent commit. In that case, the tree_entry for the branch looks like: .versions[1].sha1 = $some_sha1 .tree = <tree structure loaded from $some_sha1> However, when trees are imported, rather than inherited, that is not the case. One can import a tree with a filemodify command, replacing the root tree object. e.g. "M 040000 $some_sha1 \n" In this case, the tree_entry for the branch looks like: .versions[1].sha1 = $some_sha1 .tree = NULL When adding new notes with the notemodify command, do_change_note_fanout is called to get a notes count, and to do so, it loops over the tree_entry->tree, but doesn't do anything when the tree is NULL. In the latter case above, it means do_change_note_fanout thinks the tree contains no notes, and new notes are added with no fanout. Interestingly, do_change_note_fanout does check whether subdirectories have a NULL .tree, in which case it uses load_tree(). Which means the right behaviour happens when using the filemodify command to import subdirectories. This change makes do_change_note_fanount call load_tree() whenever the tree_entry it is given has no tree loaded, making all cases handled equally. Signed-off-by: Mike Hommey <mh@glandium.org> Reviewed-by: Johan Herland <johan@herland.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-11-15compression: unify pack.compression configuration parsingLibravatar Junio C Hamano1-13/+0
There are three codepaths that use a variable whose name is pack_compression_level to affect how objects and deltas sent to a packfile is compressed. Unlike zlib_compression_level that controls the loose object compression, however, this variable was static to each of these codepaths. Two of them read the pack.compression configuration variable, using core.compression as the default, and one of them also allowed overriding it from the command line. The other codepath in bulk-checkin did not pay any attention to the configuration. Unify the configuration parsing to git_default_config(), where we implement the parsing of core.loosecompression and core.compression and make the former override the latter, by moving code to parse pack.compression and also allow core.compression to give default to this variable. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-09-29use QSORTLibravatar René Scharfe1-2/+2
Apply the semantic patch contrib/coccinelle/qsort.cocci to the code base, replacing calls of qsort(3) with QSORT. The resulting code is shorter and supports empty arrays with NULL pointers. Signed-off-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-07-19Merge branch 'jk/common-main'Libravatar Junio C Hamano1-7/+2
There are certain house-keeping tasks that need to be performed at the very beginning of any Git program, and programs that are not built-in commands had to do them exactly the same way as "git" potty does. It was easy to make mistakes in one-off standalone programs (like test helpers). A common "main()" function that calls cmd_main() of individual program has been introduced to make it harder to make mistakes. * jk/common-main: mingw: declare main()'s argv as const common-main: call git_setup_gettext() common-main: call restore_sigpipe_to_default() common-main: call sanitize_stdfds() common-main: call git_extract_argv0_path() add an extra level of indirection to main()
2016-07-06Merge branch 'jk/common-main-2.8' into jk/common-mainLibravatar Junio C Hamano1-7/+2
* jk/common-main-2.8: mingw: declare main()'s argv as const common-main: call git_setup_gettext() common-main: call restore_sigpipe_to_default() common-main: call sanitize_stdfds() common-main: call git_extract_argv0_path() add an extra level of indirection to main()
2016-07-01common-main: call git_setup_gettext()Libravatar Jeff King1-2/+0
This should be part of every program, as otherwise users do not get translated error messages. However, some external commands forgot to do so (e.g., git-credential-store). This fixes them, and eliminates the repeated code in programs that did remember to use it. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-07-01common-main: call git_extract_argv0_path()Libravatar Jeff King1-3/+0
Every program which links against libgit.a must call this function, or risk hitting an assert() in system_path() that checks whether we have configured argv0_path (though only when RUNTIME_PREFIX is defined, so essentially only on Windows). Looking at the diff, you can see that putting it into the common main() saves us having to do it individually in each of the external commands. But what you can't see are the cases where we _should_ have been doing so, but weren't (e.g., git-credential-store, and all of the t/helper test programs). This has been an accident-waiting-to-happen for a long time, but wasn't triggered until recently because it involves one of those programs actually calling system_path(). That happened with git-credential-store in v2.8.0 with ae5f677 (lazily load core.sharedrepository, 2016-03-11). The program: - takes a lock file, which... - opens a tempfile, which... - calls adjust_shared_perm to fix permissions, which... - lazy-loads the config (as of ae5f677), which... - calls system_path() to find the location of /etc/gitconfig On systems with RUNTIME_PREFIX, this means credential-store reliably hits that assert() and cannot be used. We never noticed in the test suite, because we set GIT_CONFIG_NOSYSTEM there, which skips the system_path() lookup entirely. But if we were to tweak git_config() to find /etc/gitconfig even when we aren't going to open it, then the test suite shows multiple failures (for credential-store, and for some other test helpers). I didn't include that tweak here because it's way too specific to this particular call to be worth carrying around what is essentially dead code. The implementation is fairly straightforward, with one exception: there is exactly one caller (git.c) that actually cares about the result of the function, and not the side-effect of setting up argv0_path. We can accommodate that by simply replacing the value of argv[0] in the array we hand down to cmd_main(). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-07-01add an extra level of indirection to main()Libravatar Jeff King1-2/+2
There are certain startup tasks that we expect every git process to do. In some cases this is just to improve the quality of the program (e.g., setting up gettext()). In others it is a requirement for using certain functions in libgit.a (e.g., system_path() expects that you have called git_extract_argv0_path()). Most commands are builtins and are covered by the git.c version of main(). However, there are still a few external commands that use their own main(). Each of these has to remember to include the correct startup sequence, and we are not always consistent. Rather than just fix the inconsistencies, let's make this harder to get wrong by providing a common main() that can run this standard startup. We basically have two options to do this: - the compat/mingw.h file already does something like this by adding a #define that replaces the definition of main with a wrapper that calls mingw_startup(). The upside is that the code in each program doesn't need to be changed at all; it's rewritten on the fly by the preprocessor. The downside is that it may make debugging of the startup sequence a bit more confusing, as the preprocessor is quietly inserting new code. - the builtin functions are all of the form cmd_foo(), and git.c's main() calls them. This is much more explicit, which may make things more obvious to somebody reading the code. It's also more flexible (because of course we have to figure out _which_ cmd_foo() to call). The downside is that each of the builtins must define cmd_foo(), instead of just main(). This patch chooses the latter option, preferring the more explicit approach, even though it is more invasive. We introduce a new file common-main.c, with the "real" main. It expects to call cmd_main() from whatever other objects it is linked against. We link common-main.o against anything that links against libgit.a, since we know that such programs will need to do this setup. Note that common-main.o can't actually go inside libgit.a, as the linker would not pick up its main() function automatically (it has no callers). The rest of the patch is just adjusting all of the various external programs (mostly in t/helper) to use cmd_main(). I've provided a global declaration for cmd_main(), which means that all of the programs also need to match its signature. In particular, many functions need to switch to "const char **" instead of "char **" for argv. This effect ripples out to a few other variables and functions, as well. This makes the patch even more invasive, but the end result is much better. We should be treating argv strings as const anyway, and now all programs conform to the same signature (which also matches the way builtins are defined). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-06-20Merge branch 'ew/fast-import-unpack-limit'Libravatar Junio C Hamano1-0/+61
"git fast-import" learned the same performance trick to avoid creating too small a packfile as "git fetch" and "git push" have, using *.unpackLimit configuration. * ew/fast-import-unpack-limit: fast-import: invalidate pack_id references after loosening fast-import: implement unpack limit
2016-05-31Merge branch 'fc/fast-import-broken-marks-file'Libravatar Junio C Hamano1-2/+5
"git fast-import --export-marks" would overwrite the existing marks file even when it makes a dump from its custom die routine. Prevent it from doing so when we have an import-marks file but haven't finished reading it. * fc/fast-import-broken-marks-file: fast-import: do not truncate exported marks file
2016-05-29fast-import: invalidate pack_id references after looseningLibravatar Eric Wong1-1/+30
When loosening a pack, the current pack_id gets reused when checkpointing and the import does not terminate. This causes problems after checkpointing as the object table, branch, and tag lists still contains pre-checkpoint references to the recycled pack_id. Merely clearing the object_table as suggested by Jeff King in http://mid.gmane.org/20160517121330.GA7346@sigill.intra.peff.net is insufficient as the marks set still contains references to object entries. Wrong pack_id references branch and tags lists do not cause errors, but can lead to misleading crash reports and core dumps, so they are also invalidated. Signed-off-by: Eric Wong <e@80x24.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-05-23Merge branch 'nd/worktree-various-heads'Libravatar Junio C Hamano1-3/+3
The experimental "multiple worktree" feature gains more safety to forbid operations on a branch that is checked out or being actively worked on elsewhere, by noticing that e.g. it is being rebased. * nd/worktree-various-heads: branch: do not rename a branch under bisect or rebase worktree.c: check whether branch is bisected in another worktree wt-status.c: split bisect detection out of wt_status_get_state() worktree.c: check whether branch is rebased in another worktree worktree.c: avoid referencing to worktrees[i] multiple times wt-status.c: make wt_status_check_rebase() work on any worktree wt-status.c: split rebase detection out of wt_status_get_state() path.c: refactor and add worktree_git_path() worktree.c: mark current worktree worktree.c: make find_shared_symref() return struct worktree * worktree.c: store "id" instead of "git_dir" path.c: add git_common_path() and strbuf_git_common_path() dir.c: rename str(n)cmp_icase to fspath(n)cmp
2016-05-17fast-import: do not truncate exported marks fileLibravatar Felipe Contreras1-2/+5
Certain lines of the marks file might be corrupted (or the objects missing due to a garbage collection), but that's no reason to truncate the file and essentially destroy the rest of it. Ideally missing objects should not cause a crash, we could just skip them, but that's another patch. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-05-11fast-import: implement unpack limitLibravatar Eric Wong1-0/+32
With many incremental imports, small packs become highly inefficient due to the need to readdir scan and load many indices to locate even a single object. Frequent repacking and consolidation may be prohibitively expensive in terms of disk I/O, especially in large repositories where the initial packs were aggressively optimized and marked with .keep files. In those cases, users may be better served with loose objects and relying on "git gc --auto". This changes the default behavior of fast-import for small imports found in test cases, so adjustments to t9300 were necessary. Signed-off-by: Eric Wong <normalperson@yhbt.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-05-09fast-import.c: use error_errno()Libravatar Nguyễn Thái Ngọc Duy1-5/+5
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-04-22dir.c: rename str(n)cmp_icase to fspath(n)cmpLibravatar Nguyễn Thái Ngọc Duy1-3/+3
These functions compare two paths that are taken from file system. Depending on the running file system, paths may need to be compared case-sensitively or not, and maybe even something else in future. The current names do not convey that well. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-02-26Merge branch 'jk/tighten-alloc'Libravatar Junio C Hamano1-7/+5
Update various codepaths to avoid manually-counted malloc(). * jk/tighten-alloc: (22 commits) ewah: convert to REALLOC_ARRAY, etc convert ewah/bitmap code to use xmalloc diff_populate_gitlink: use a strbuf transport_anonymize_url: use xstrfmt git-compat-util: drop mempcpy compat code sequencer: simplify memory allocation of get_message test-path-utils: fix normalize_path_copy output buffer size fetch-pack: simplify add_sought_entry fast-import: simplify allocation in start_packfile write_untracked_extension: use FLEX_ALLOC helper prepare_{git,shell}_cmd: use argv_array use st_add and st_mult for allocation size computation convert trivial cases to FLEX_ARRAY macros use xmallocz to avoid size arithmetic convert trivial cases to ALLOC_ARRAY convert manual allocations to argv_array argv-array: add detach function add helpers for allocating flex-array structs harden REALLOC_ARRAY and xcalloc against size_t overflow tree-diff: catch integer overflow in combine_diff_path allocation ...
2016-02-22fast-import: simplify allocation in start_packfileLibravatar Jeff King1-4/+1
This function allocate a packed_git flex-array, and adds a mysterious 2 bytes to the length of the pack_name field. One is for the trailing NUL, but the other has no purpose. This is probably cargo-culted from add_packed_git, which gets the ".idx" path and needed to allocate enough space to hold the matching ".pack" (though since 48bcc1c, we calculate the size there differently). This site, however, is using the raw path of a tempfile, and does not need the extra byte. We can just replace the allocation with FLEX_ALLOC_STR, which handles the allocation and the NUL for us. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-02-22use st_add and st_mult for allocation size computationLibravatar Jeff King1-1/+1
If our size computation overflows size_t, we may allocate a much smaller buffer than we expected and overflow it. It's probably impossible to trigger an overflow in most of these sites in practice, but it is easy enough convert their additions and multiplications into overflow-checking variants. This may be fixing real bugs, and it makes auditing the code easier. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-02-22convert trivial cases to ALLOC_ARRAYLibravatar Jeff King1-2/+3
Each of these cases can be converted to use ALLOC_ARRAY or REALLOC_ARRAY, which has two advantages: 1. It automatically checks the array-size multiplication for overflow. 2. It always uses sizeof(*array) for the element-size, so that it can never go out of sync with the declared type of the array. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2016-01-15strbuf: introduce strbuf_getline_{lf,nul}()Libravatar Junio C Hamano1-2/+2
The strbuf_getline() interface allows a byte other than LF or NUL as the line terminator, but this is only because I wrote these codepaths anticipating that there might be a value other than NUL and LF that could be useful when I introduced line_termination long time ago. No useful caller that uses other value has emerged. By now, it is clear that the interface is overly broad without a good reason. Many codepaths have hardcoded preference to read either LF terminated or NUL terminated records from their input, and then call strbuf_getline() with LF or NUL as the third parameter. This step introduces two thin wrappers around strbuf_getline(), namely, strbuf_getline_lf() and strbuf_getline_nul(), and mechanically rewrites these call sites to call either one of them. The changes contained in this patch are: * introduction of these two functions in strbuf.[ch] * mechanical conversion of all callers to strbuf_getline() with either '\n' or '\0' as the third parameter to instead call the respective thin wrapper. After this step, output from "git grep 'strbuf_getline('" would become a lot smaller. An interim goal of this series is to make this an empty set, so that we can have strbuf_getline_crlf() take over the shorter name strbuf_getline(). Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-12-11Merge branch 'sg/lock-file-commit-error'Libravatar Junio C Hamano1-1/+1
Cosmetic improvement to lock-file error messages. * sg/lock-file-commit-error: Make error message after failing commit_lock_file() less confusing
2015-12-01Make error message after failing commit_lock_file() less confusingLibravatar SZEDER Gábor1-1/+1
The error message after a failing commit_lock_file() call sometimes looks like this, causing confusion: $ git remote add remote git@server.com/repo.git error: could not commit config file .git/config # Huh?! # I didn't want to commit anything, especially not my config file! While in the narrow context of the lockfile module using the verb 'commit' in the error message makes perfect sense, in the broader context of git the word 'commit' already has a very specific meaning, hence the confusion. Reword these error messages to say "could not write" instead of "could not commit". While at it, include strerror in the error messages after writing the config file or the credential store fails to provide some information about the cause of the failure, and update the style of the error message after writing the reflog fails to match surrounding error messages (i.e. no '' around the pathname and no () around the error description). Signed-off-by: SZEDER Gábor <szeder@ira.uka.de> Signed-off-by: Jeff King <peff@peff.net>
2015-10-20Merge branch 'jk/war-on-sprintf'Libravatar Junio C Hamano1-7/+10
Many allocations that is manually counted (correctly) that are followed by strcpy/sprintf have been replaced with a less error prone constructs such as xstrfmt. Macintosh-specific breakage was noticed and corrected in this reroll. * jk/war-on-sprintf: (70 commits) name-rev: use strip_suffix to avoid magic numbers use strbuf_complete to conditionally append slash fsck: use for_each_loose_file_in_objdir Makefile: drop D_INO_IN_DIRENT build knob fsck: drop inode-sorting code convert strncpy to memcpy notes: document length of fanout path with a constant color: add color_set helper for copying raw colors prefer memcpy to strcpy help: clean up kfmclient munging receive-pack: simplify keep_arg computation avoid sprintf and strcpy with flex arrays use alloc_ref rather than hand-allocating "struct ref" color: add overflow checks for parsing colors drop strcpy in favor of raw sha1_to_hex use sha1_to_hex_r() instead of strcpy daemon: use cld->env_array when re-spawning stat_tracking_info: convert to argv_array http-push: use an argv_array for setup_revisions fetch-pack: use argv_array for index-pack / unpack-objects ...
2015-10-05Merge branch 'jk/date-local'Libravatar Junio C Hamano1-1/+1
"git log --date=local" used to only show the normal (default) format in the local timezone. The command learned to take 'local' as an instruction to use the local timezone with other formats, e.g. "git show --date=rfc-local". * jk/date-local: t6300: add tests for "-local" date formats t6300: make UTC and local dates different date: make "local" orthogonal to date format date: check for "local" before anything else t6300: add test for "raw" date format t6300: introduce test_date() helper fast-import: switch crash-report date to iso8601 Documentation/rev-list: don't list date formats Documentation/git-for-each-ref: don't list date formats Documentation/config: don't list date formats Documentation/blame-options: don't list date formats
2015-10-05convert strncpy to memcpyLibravatar Jeff King1-1/+1
strncpy is known to be a confusing function because of its termination semantics. These calls are all correct, but it takes some examination to see why. In particular, every one of them expects to copy up to the length limit, and then makes some arrangement for terminating the result. We can just use memcpy, along with noting explicitly how the result is terminated (if it is not already obvious). That should make it more clear to a reader that we are doing the right thing. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-10-05prefer memcpy to strcpyLibravatar Jeff King1-2/+3
When we already know the length of a string (e.g., because we just malloc'd to fit it), it's nicer to use memcpy than strcpy, as it makes it more obvious that we are not going to overflow the buffer (because the size we pass matches the size in the allocation). This also eliminates calls to strcpy, which make auditing the code base harder. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-10-05avoid sprintf and strcpy with flex arraysLibravatar Jeff King1-2/+4
When we are allocating a struct with a FLEX_ARRAY member, we generally compute the size of the array and then sprintf or strcpy into it. Normally we could improve a dynamic allocation like this by using xstrfmt, but it doesn't work here; we have to account for the size of the rest of the struct. But we can improve things a bit by storing the length that we use for the allocation, and then feeding it to xsnprintf or memcpy, which makes it more obvious that we are not writing more than the allocated number of bytes. It would be nice if we had some kind of helper for allocating generic flex arrays, but it doesn't work that well: - the call signature is a little bit unwieldy: d = flex_struct(sizeof(*d), offsetof(d, path), fmt, ...); You need offsetof here instead of just writing to the end of the base size, because we don't know how the struct is packed (partially this is because FLEX_ARRAY might not be zero, though we can account for that; but the size of the struct may actually be rounded up for alignment, and we can't know that). - some sites do clever things, like over-allocating because they know they will write larger things into the buffer later (e.g., struct packed_git here). So we're better off to just write out each allocation (or add type-specific helpers, though many of these are one-off allocations anyway). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-09-25use xsnprintf for generating git object headersLibravatar Jeff King1-2/+2
We generally use 32-byte buffers to format git's "type size" header fields. These should not generally overflow unless you can produce some truly gigantic objects (and our types come from our internal array of constant strings). But it is a good idea to use xsnprintf to make sure this is the case. Note that we slightly modify the interface to write_sha1_file_prepare, which nows uses "hdrlen" as an "in" parameter as well as an "out" (on the way in it stores the allocated size of the header, and on the way out it returns the ultimate size of the header). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-09-03fast-import: switch crash-report date to iso8601Libravatar Jeff King1-1/+1
When fast-import emits a crash report, it does so in the user's local timezone. But because we omit the timezone completely for DATE_LOCAL, a reader of the report does not immediately know which time zone was used. Let's switch this to ISO8601 instead, which includes the time zone. This does mean we will show the time in UTC, but that's not a big deal. A crash report like this will either be looked at immediately (in which case nobody even looks at the timestamp), or it will be passed along to a developer to debug, in which case the original timezone is less likely to be of interest. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: John Keeping <john@keeping.me.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-19Merge branch 'jk/git-path'Libravatar Junio C Hamano1-1/+3
git_path() and mkpath() are handy helper functions but it is easy to misuse, as the callers need to be careful to keep the number of active results below 4. Their uses have been reduced. * jk/git-path: memoize common git-path "constant" files get_repo_path: refactor path-allocation find_hook: keep our own static buffer refs.c: remove_empty_directories can take a strbuf refs.c: avoid git_path assignment in lock_ref_sha1_basic refs.c: avoid repeated git_path calls in rename_tmp_log refs.c: simplify strbufs in reflog setup and writing path.c: drop git_path_submodule refs.c: remove extra git_path calls from read_loose_refs remote.c: drop extraneous local variable from migrate_file prefer mkpathdup to mkpath in assignments prefer git_pathdup to git_path in some possibly-dangerous cases add_to_alternates_file: don't add duplicate entries t5700: modernize style cache.h: complete set of git_path_submodule helpers cache.h: clarify documentation for git_path, et al
2015-08-19Merge branch 'jc/finalize-temp-file'Libravatar Junio C Hamano1-2/+2
Long overdue micro clean-up. * jc/finalize-temp-file: sha1_file.c: rename move_temp_to_file() to finalize_object_file()
2015-08-10prefer git_pathdup to git_path in some possibly-dangerous casesLibravatar Jeff King1-1/+3
Because git_path uses a static buffer that is shared with calls to git_path, mkpath, etc, it can be dangerous to assign the result to a variable or pass it to a non-trivial function. The value may change unexpectedly due to other calls. None of the cases changed here has a known bug, but they're worth converting away from git_path because: 1. It's easy to use git_pathdup in these cases. 2. They use constructs (like assignment) that make it hard to tell whether they're safe or not. The extra malloc overhead should be trivial, as an allocation should be an order of magnitude cheaper than a system call (which we are clearly about to make, since we are constructing a filename). The real cost is that we must remember to free the result. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2015-08-10sha1_file.c: rename move_temp_to_file() to finalize_object_file()Libravatar Junio C Hamano1-2/+2
Since 5a688fe4 ("core.sharedrepository = 0mode" should set, not loosen, 2009-03-25), we kept reminding ourselves: NEEDSWORK: this should be renamed to finalize_temp_file() as "moving" is only a part of what it does, when no patch between master to pu changes the call sites of this function. without doing anything about it. Let's do so. The purpose of this function was not to move but to finalize. The detail of the primarily implementation of finalizing was to link the temporary file to its final name and then to unlink, which wasn't even "moving". The alternative implementation did "move" by calling rename(2), which is a fun tangent. Signed-off-by: Junio C Hamano <gitster@pobox.com>