kernel/git - git - PowerEL Git System

Commit Graph

Author	SHA1	Message	Date
Jeff King	10ecfa7649	verify_path: disallow symlinks in .gitmodules There are a few reasons it's not a good idea to make .gitmodules a symlink, including: 1. It won't be portable to systems without symlinks. 2. It may behave inconsistently, since Git may look at this file in the index or a tree without bothering to resolve any symbolic links. We don't do this _yet_, but the config infrastructure is there and it's planned for the future. With some clever code, we could make (2) work. And some people may not care about (1) if they only work on one platform. But there are a few security reasons to simply disallow it: a. A symlinked .gitmodules file may circumvent any fsck checks of the content. b. Git may read and write from the on-disk file without sanity checking the symlink target. So for example, if you link ".gitmodules" to "../oops" and run "git submodule add", we'll write to the file "oops" outside the repository. Again, both of those are problems that _could_ be solved with sufficient code, but given the complications in (1) and (2), we're better off just outlawing it explicitly. Note the slightly tricky call to verify_path() in update-index's update_one(). There we may not have a mode if we're not updating from the filesystem (e.g., we might just be removing the file). Passing "0" as the mode there works fine; since it's not a symlink, we'll just skip the extra checks. Signed-off-by: Jeff King <peff@peff.net>	7 years ago
Jeff King	eb12dd0c76	update-index: stat updated files earlier In the update_one(), we check verify_path() on the proposed path before doing anything else. In preparation for having verify_path() look at the file mode, let's stat the file earlier, so we can check the mode accurately. This is made a bit trickier by the fact that this function only does an lstat in a few code paths (the ones that flow down through process_path()). So we can speculatively do the lstat() here and pass the results down, and just use a dummy mode for cases where we won't actually be updating the index from the filesystem. Signed-off-by: Jeff King <peff@peff.net>	7 years ago
Jeff King	0383bbb901	submodule-config: verify submodule names as paths Submodule "names" come from the untrusted .gitmodules file, but we blindly append them to $GIT_DIR/modules to create our on-disk repo paths. This means you can do bad things by putting "../" into the name (among other things). Let's sanity-check these names to avoid building a path that can be exploited. There are two main decisions: 1. What should the allowed syntax be? It's tempting to reuse verify_path(), since submodule names typically come from in-repo paths. But there are two reasons not to: a. It's technically more strict than what we need, as we really care only about breaking out of the $GIT_DIR/modules/ hierarchy. E.g., having a submodule named "foo/.git" isn't actually dangerous, and it's possible that somebody has manually given such a funny name. b. Since we'll eventually use this checking logic in fsck to prevent downstream repositories, it should be consistent across platforms. Because verify_path() relies on is_dir_sep(), it wouldn't block "foo\..\bar" on a non-Windows machine. 2. Where should we enforce it? These days most of the .gitmodules reads go through submodule-config.c, so I've put it there in the reading step. That should cover all of the C code. We also construct the name for "git submodule add" inside the git-submodule.sh script. This is probably not a big deal for security since the name is coming from the user anyway, but it would be polite to remind them if the name they pick is invalid (and we need to expose the name-checker to the shell anyway for our test scripts). This patch issues a warning when reading .gitmodules and just ignores the related config entry completely. This will generally end up producing a sensible error, as it works the same as a .gitmodules file which is missing a submodule entry (so "submodule update" will barf, but "git clone --recurse-submodules" will print an error but not abort the clone. There is one minor oddity, which is that we print the warning once per malformed config key (since that's how the config subsystem gives us the entries). So in the new test, for example, the user would see three warnings. That's OK, since the intent is that this case should never come up outside of malicious repositories (and then it might even benefit the user to see the message multiple times). Credit for finding this vulnerability and the proof of concept from which the test script was adapted goes to Etienne Stalmans. Signed-off-by: Jeff King <peff@peff.net>	7 years ago
Shuyu Wei	ffb4568afe	pull: pass -4/-6 option to 'git fetch' The -4/-6 option should be passed through to 'git fetch' to be consistent with the man page. Signed-off-by: Wei Shuyu <wsy@dogben.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Todd Zullinger	89b9e31dd5	notes: send "Automatic notes merge failed" messages to stderr All other error messages from notes use stderr. Do the same when alerting users of an unresolved notes merge. Fix the output redirection in t3310 and t3320 as well. Previously, the tests directed output to a file, but stderr was either not captured or not sent to the file due to the order of the redirection operators. Signed-off-by: Todd Zullinger <tmz@pobox.com> Acked-by: Johan Herland <johan@herland.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Martin Ågren	4da72644b7	reduce_heads: fix memory leaks We currently have seven callers of `reduce_heads(foo)`. Six of them do not use the original list `foo` again, and actually, all six of those end up leaking it. Introduce and use `reduce_heads_replace(&foo)` as a leak-free version of `foo = reduce_heads(foo)` to fix several of these. Fix the remaining leaks using `free_commit_list()`. While we're here, document `reduce_heads()` and mark it as `extern`. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Martin Ågren	a452d0f4ba	builtin/merge-base: free commit lists In several functions, we iterate through a commit list by assigning `result = result->next`. As a consequence, we lose the original pointer and eventually leak the list. Rewrite the loops so that we keep the original pointers, then call `free_commit_list()`. Various alternatives were considered: 1) Use `UNLEAK(result)` before the loop. Simple change, but not very pretty. These would definitely be new lows among our usages of UNLEAK. 2) Use `pop_commit()` when looping. Slightly less simple change, but it feels slightly preferable to first display the list, then free it. 3) As in this patch, but with `UNLEAK()` instead of freeing. We'd still go through all the trouble of refactoring the loop, and because it's not super-obvious that we're about to exit, let's just free the lists -- it probably doesn't affect the runtime much. In `handle_independent()` we can drop `result` while we're here and reuse the `revs`-variable instead. That matches several other users of `reduce_heads()`. The memory-leak that this hides will be addressed in the next commit. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Martin Ågren	24d707f636	bisect: change calling-convention of `find_bisection()` This function takes a commit list and returns a commit list. The returned list is built by modifying the original list. Thus the caller should not use the original list again (and after the next commit fixes a memory leak, it must not). Change the function signature so that it takes a **list and has void return type. That should make it harder to misuse this function. While we're here, document this function. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Martin Ågren	9560e6245a	grep: take the read-lock when adding a submodule With --recurse-submodules, we add each submodule that we encounter to the list of alternate object databases. With threading, our changes to the list are not protected against races. Indeed, ThreadSanitizer reports a race when we call `add_to_alternates_memory()` around the same time that another thread is reading in the list through `read_sha1_file()`. Take the grep read-lock while adding the submodule. The lock is used to serialize uses of non-thread-safe parts of Git's API, including `read_sha1_file()`. Helped-by: Brandon Williams <bmwill@google.com> Signed-off-by: Martin Ågren <martin.agren@gmail.com> Acked-by: Brandon Williams <bmwill@google.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	752848df0f	remote: handle broken symrefs It's possible for resolve_ref_unsafe() to return NULL with a REF_ISSYMREF flag if a symref points to a broken ref. In this case, the read_remote_branches() function will segfault passing the name to xstrdup(). This is hard to trigger in practice, since this function is used as a callback to for_each_ref(), which will skip broken refs in the first place (so it would have to be broken racily, or for us to see a transient filesystem error). If we see such a racy broken outcome let's treat it as "not a symref". This is exactly the same thing that would happen in the non-racy case (our function would not be called at all, as for_each_ref would skip the broken symref). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Andrey Okoshkin	c26de08370	commit: check result of resolve_ref_unsafe Add check of the resolved HEAD reference while printing of a commit summary. resolve_ref_unsafe() may return NULL pointer if underlying calls of lstat() or open() fail in files_read_raw_ref(). Such situation can be caused by race: file becomes inaccessible to this moment. Signed-off-by: Andrey Okoshkin <a.okoshkin@samsung.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Junio C Hamano	7ccc94ff45	check-ref-format --branch: strip refs/heads/ using skip_prefix The expansion returned from strbuf_check_branch_ref always starts with "refs/heads/" by construction, but there is nothing about its name or advertised API making that obvious. This command is used to process human-supplied input from the command line and is usually not the inner loop, so we can spare some cycles to be more defensive. Instead of hard-coding the offset strlen("refs/heads/") to skip, verify that the expansion actually starts with refs/heads/. [jn: split out from a larger patch, added explanation] Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	b521fd1228	tag: respect color.ui config Since `11b087adfd` (ref-filter: consult want_color() before emitting colors, 2017-07-13), we expect that setting "color.ui" to "always" will enable color tag formats even without a tty. As that commit was built on top of `136c8c8b8f` (color: check color.ui in git_default_config(), 2017-07-13) from the same series, we didn't need to touch tag's config parsing at all. However, since we reverted `136c8c8b8f`, we now need to explicitly call git_color_default_config() to make this work. Let's do so, and also restore the test dropped in `0c88bf5050` (provide --color option for all ref-filter users, 2017-10-03). That commit swapped out our "color.ui=always" test for "--color" in preparation for "always" going away. But since it is here to stay, we should test both cases. Note that for-each-ref also lost its color.ui support as part of reverting `136c8c8b8f`. But as a plumbing command, it should _not_ respect the color.ui config. Since it also gained a --color option in `0c88bf5050`, that's the correct way to ask it for color. We'll continue to test that, and confirm that "color.ui" is not respected. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	33c643bb08	Revert "color: check color.ui in git_default_config()" This reverts commit `136c8c8b8f`. That commit was trying to address a bug caused by `4c7f1819b3` (make color.ui default to 'auto', 2013-06-10), in which plumbing like diff-tree defaulted to "auto" color, but did not respect a "color.ui" directive to disable it. But it also meant that we started respecting "color.ui" set to "always". This was a known problem, but `4c7f1819b3` argued that nobody ought to be doing that. However, that turned out to be wrong, and we got a number of bug reports related to "add -p" regressing in v2.14.2. Let's revert `136c8c8b8`, fixing the regression to "add -p". This leaves the problem from `4c7f1819b3` unfixed, but: 1. It's a pretty obscure problem in the first place. I only noticed it while working on the color code, and we haven't got a single bug report or complaint about it. 2. We can make a more moderate fix on top by respecting "never" but not "always" for plumbing commands. This is just the minimal fix to go back to the working state we had before v2.14.2. Note that this isn't a pure revert. We now have a test in t3701 which shows off the "add -p" regression. This can be flipped to success. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Derrick Stolee	19716b21a4	cleanup: fix possible overflow errors in binary search A common mistake when writing binary search is to allow possible integer overflow by using the simple average: mid = (min + max) / 2; Instead, use the overflow-safe version: mid = min + (max - min) / 2; This translation is safe since the operation occurs inside a loop conditioned on "min < max". The included changes were found using the following git grep: git grep '/ 2;' '.c' Making this cleanup will prevent future review friction when a new binary search is contructed based on existing code. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Stefan Beller	a9155c50bd	branch: reset instead of release a strbuf Our documentation advises to not re-use a strbuf, after strbuf_release has been called on it. Use the proper reset instead. Currently 'strbuf_release' releases and re-initializes the strbuf, so it is safe, but slow. 'strbuf_reset' only resets the internal length variable, such that this could also be accounted for as a micro-optimization. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Kaartic Sivaraam	f777623514	branch: change the error messages to be more meaningful The error messages shown when the branch command is misused by supplying it wrong number of parameters wasn't meaningful. That's because it used the the phrase "too many branches" assuming all parameters to be "valid" branch names. It's not always the case as exemplified below, $ git branch foo * master $ git branch -m foo foo old fatal: too many branches for a rename operation Change the messages to be more general thus making no assumptions about the "parameters". Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	0c88bf5050	provide --color option for all ref-filter users When ref-filter learned about want_color() in `11b087adfd` (ref-filter: consult want_color() before emitting colors, 2017-07-13), it became useful to be able to turn colors off and on for specific commands. For git-branch, you can do so with --color/--no-color. But for git-for-each-ref and git-tag, the other users of ref-filter, you have no option except to tweak the "color.ui" config setting. Let's give both of these commands the usual color command-line options. This is a bit more obvious as a method for overriding the config. And it also prepares us for the behavior of "always" changing (so that we are still left with a way of forcing color when our output goes to a non-terminal). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
René Scharfe	72d4a9a721	use strbuf_addstr() for adding strings to strbufs Use strbuf_addstr() instead of strbuf_addf() for adding strings. That's simpler and makes the intent clearer. Patch generated by Coccinelle and contrib/coccinelle/strbuf.cocci; adjusted indentation in refs/packed-backend.c manually. Signed-off-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Martin Ågren	886e1084d7	builtin/: add UNLEAKs Add some UNLEAKs where we are about to return from `cmd_*`. UNLEAK the variables in the same order as we've declared them. While addressing `msg` in builtin/tag.c, convert the existing `strbuf_release()` calls as well. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
René Scharfe	efbd4fdfc9	refs: pass NULL to resolve_refdup() if hash is not needed This allows us to get rid of several write-only variables. Signed-off-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	27344d6a6c	git: add --no-optional-locks option Some tools like IDEs or fancy editors may periodically run commands like "git status" in the background to keep track of the state of the repository. Some of these commands may refresh the index and write out the result in an opportunistic way: if they can get the index lock, then they update the on-disk index with any updates they find. And if not, then their in-core refresh is lost and just has to be recomputed by the next caller. But taking the index lock may conflict with other operations in the repository. Especially ones that the user is doing themselves, which _aren't_ opportunistic. In other words, "git status" knows how to back off when somebody else is holding the lock, but other commands don't know that status would be happy to drop the lock if somebody else wanted it. There are a couple possible solutions: 1. Have some kind of "pseudo-lock" that allows other commands to tell status that they want the lock. This is likely to be complicated and error-prone to implement (and maybe even impossible with just dotlocks to work from, as it requires some inter-process communication). 2. Avoid background runs of commands like "git status" that want to do opportunistic updates, preferring instead plumbing like diff-files, etc. This is awkward for a couple of reasons. One is that "status --porcelain" reports a lot more about the repository state than is available from individual plumbing commands. And two is that we actually _do_ want to see the refreshed index. We just don't want to take a lock or write out the result. Whereas commands like diff-files expect us to refresh the index separately and write it to disk so that they can depend on the result. But that write is exactly what we're trying to avoid. 3. Ask "status" not to lock or write the index. This is easy to implement. The big downside is that any work done in refreshing the index for such a call is lost when the process exits. So a background process may end up re-hashing a changed file multiple times until the user runs a command that does an index refresh themselves. This patch implements the option 3. The idea (and the test) is largely stolen from a Git for Windows patch by Johannes Schindelin, 67e5ce7f63 (status: offer not to lock the index and update it, 2016-08-12). The twist here is that instead of making this an option to "git status", it becomes a "git" option and matching environment variable. The reason there is two-fold: 1. An environment variable is carried through to sub-processes. And whether an invocation is a background process or not should apply to the whole process tree. So you could do "git --no-optional-locks foo", and if "foo" is a script or alias that calls "status", you'll still get the effect. 2. There may be other programs that want the same treatment. I've punted here on finding more callers to convert, since "status" is the obvious one to call as a repeated background job. But "git diff"'s opportunistic refresh of the index may be a good candidate. The test is taken from 67e5ce7f63, and it's worth repeating Johannes's explanation: Note that the regression test added in this commit does not really verify that no index.lock file was written; that test is not possible in a portable way. Instead, we verify that .git/index is rewritten only when `git status` is run without `--no-optional-locks`. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	8a1a8d2ad1	worktree: check the result of read_in_full() We try to read "len" bytes into a buffer and just assume that it happened correctly. In practice this should usually be the case, since we just stat'd the file to get the length. But we could be fooled by transient errors or by other processes racily truncating the file. Let's be more careful. There's a slim chance this could catch a real error, but it also prevents people and tools from getting worried while reading the code. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	228740b67b	worktree: use xsize_t to access file size To read the "gitdir" file into memory, we stat the file and allocate a buffer. But we store the size in an "int", which may be truncated. We should use a size_t and xsize_t(), which will detect truncation. An overflow is unlikely for a "gitdir" file, but it's a good practice to model. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	41dcc4dccc	distinguish error versus short read from read_in_full() Many callers of read_in_full() expect to see the exact number of bytes requested, but their error handling lumps together true read errors and short reads due to unexpected EOF. We can give more specific error messages by separating these cases (showing errno when appropriate, and otherwise describing the short read). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	61d36330b4	prefer "!=" when checking read_in_full() result Comparing the result of read_in_full() using less-than is potentially dangerous, as a negative return value may be converted to an unsigned type and be considered a success. This is discussed further in 561598cfcf (read_pack_header: handle signed/unsigned comparison in read result, 2017-09-13). Each of these instances is actually fine in practice: - in get-tar-commit-id, the HEADERSIZE macro expands to a signed integer. If it were switched to an unsigned type (e.g., a size_t), then it would be a bug. - the other two callers check for a short read only after handling a negative return separately. This is a fine practice, but we'd prefer to model "!=" as a general rule. So all of these cases can be considered cleanups and not actual bugfixes. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
René Scharfe	744c040b19	refs: pass NULL to resolve_ref_unsafe() if hash is not needed 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>	8 years ago
Martin Ågren	7199203937	object_array: add and use `object_array_pop()` 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>	8 years ago
Martin Ågren	dcb572ab94	object_array: use `object_array_clear()`, not `free()` 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>	8 years ago
Martin Ågren	b2ccdf7fc1	leak_pending: use `object_array_clear()`, not `free()` 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>	8 years ago
Martin Ågren	dd1055ed59	builtin/commit: fix memory leak in `prepare_index()` 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>	8 years ago
Junio C Hamano	e5435ff1fc	branch: fix "copy" to never touch HEAD 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>	8 years ago
Ramsay Jones	071bcaab64	ALLOC_GROW: avoid -Wsign-compare warnings Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jeff King	1cf01a34ea	consistently use "fallthrough" comments in switches 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>	8 years ago
Jeff King	cc0ea7c9e5	cat-file: handle NULL object_context.path 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>	8 years ago
Jonathan Tan	b3e8ca89cf	fast-export: do not copy from modified file When run with the "-C" option, fast-export writes 'C' commands in its output whenever the internal diff mechanism detects a file copy, indicating that fast-import should copy the given existing file to the given new filename. However, the diff mechanism works against the prior version of the file, whereas fast-import uses whatever is current. This causes issues when a commit both modifies a file and uses it as the source for a copy. Therefore, teach fast-export to refrain from writing 'C' when it has already written a modification command for a file. An existing test in t9350-fast-export is also fixed in this patch. The existing line "C file6 file7" copies the wrong version of file6, but it has coincidentally worked because file7 was subsequently overridden. Reported-by: Juraj Oršulić <juraj.orsulic@fer.hr> Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Max Kirillov	6d68b2ab78	describe: teach --match to handle branches and remotes When `git describe` uses `--match`, it matches only tags, basically ignoring the `--all` argument even when it is specified. Fix it by also matching branch name and $remote_name/$remote_branch_name, for remote-tracking references, with the specified patterns. Update documentation accordingly and add tests. Signed-off-by: Max Kirillov <max@max630.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Øystein Walle	417abfde35	rev-parse: rev-parse: add --is-shallow-repository Running `git fetch --unshallow` on a repo that is not in fact shallow produces a fatal error message. Add a helper to rev-parse that scripters can use to determine whether a repo is shallow or not. Signed-off-by: Øystein Walle <oystwa@gmail.com> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Brandon Casey	33e75122f4	rev-parse parseopt: interpret any whitespace as start of help text Currently, rev-parse only interprets a space ' ' character as the delimiter between the option spec and the help text. So if a tab character is placed between the option spec and the help text, it will be interpreted as part of the long option name or as part of the arg hint. If it is interpreted as part of the long option name, then rev-parse will produce what will be interpreted as multiple arguments on the command line. For example, the following option spec (note: there is a <tab> between "frotz" and "enable"): frotz enable frotzing will produce the following set expression when --frotz is used: set -- --frotz -- instead of this: set -- --frotz enable -- Mark t1502.2 as fixed. Signed-off-by: Brandon Casey <drafnel@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Brandon Casey	28a8d0f77a	rev-parse parseopt: do not search help text for flag chars When searching for flag characters in the option spec, we should ensure the search stays within the bounds of the option spec and does not enter the help text portion of the spec. So when we find the boundary white space marking the start of the help text, let's mark it with a nul character. Then when we search for flag characters starting from the beginning of the string we'll stop at the nul and won't enter the help text. Now, the following option spec: exclame this does something! will produce this 'set' expression when --exclame is specified: set -- --exclame -- instead of this one: set -- --exclame this does something -- Mark t1502.4 and t1502.5 as fixed. Signed-off-by: Brandon Casey <drafnel@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Junio C Hamano	afe2fab72c	gc: call fscanf() with %<len>s, not %<len>c, when reading hostname Earlier in this codepath, we (ab)used "%<len>c" to read the hostname recorded in the lockfile into locking_host[HOST_NAME_MAX + 1] while substituting <len> with the actual value of HOST_NAME_MAX. This turns out to be incorrect, as it is an instruction to read exactly the specified number of bytes. Because we are trying to read at most that many bytes, we should be using "%<len>s" instead. Helped-by: A. Wilcox <awilfox@adelielinux.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Max Kirillov	da769d2986	describe: fix matching to actually match all patterns `git describe --match` with multiple patterns matches only first pattern. If it fails, next patterns are not tried. Fix it, add test cases and update existing test which has wrong expectation. Signed-off-by: Max Kirillov <max@max630.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jeff King	06f46f237a	avoid "write_in_full(fd, buf, len) != len" pattern The return value of write_in_full() is either "-1", or the requested number of bytes[1]. If we make a partial write before seeing an error, we still return -1, not a partial value. This goes back to `f6aa66cb95` (write_in_full: really write in full or return error on disk full., 2007-01-11). So checking anything except "was the return value negative" is pointless. And there are a couple of reasons not to do so: 1. It can do a funny signed/unsigned comparison. If your "len" is signed (e.g., a size_t) then the compiler will promote the "-1" to its unsigned variant. This works out for "!= len" (unless you really were trying to write the maximum size_t bytes), but is a bug if you check "< len" (an example of which was fixed recently in config.c). We should avoid promoting the mental model that you need to check the length at all, so that new sites are not tempted to copy us. 2. Checking for a negative value is shorter to type, especially when the length is an expression. 3. Linus says so. In `d34cf19b89` (Clean up write_in_full() users, 2007-01-11), right after the write_in_full() semantics were changed, he wrote: I really wish every "write_in_full()" user would just check against "<0" now, but this fixes the nasty and stupid ones. Appeals to authority aside, this makes it clear that writing it this way does not have an intentional benefit. It's a historical curiosity that we never bothered to clean up (and which was undoubtedly cargo-culted into new sites). So let's convert these obviously-correct cases (this includes write_str_in_full(), which is just a wrapper for write_in_full()). [1] A careful reader may notice there is one way that write_in_full() can return a different value. If we ask write() to write N bytes and get a return value that is _larger_ than N, we could return a larger total. But besides the fact that this would imply a totally broken version of write(), it would already invoke undefined behavior. Our internal remaining counter is an unsigned size_t, which means that subtracting too many byte will wrap it around to a very large number. So we'll instantly begin reading off the end of the buffer, trying to write gigabytes (or petabytes) of data. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jeff King	68a423ab3e	get-tar-commit-id: check write_in_full() return against 0 We ask to write 41 bytes and make sure that the return value is at least 41. This is the same "dangerous" pattern that was fixed in the prior commit (wherein a negative return value is promoted to unsigned), though it is not dangerous here because our "41" is a constant, not an unsigned variable. But we should convert it anyway to avoid modeling a dangerous construct. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jonathan Nieder	607bd8315c	pack: make packed_git_mru global a value instead of a pointer The MRU cache that keeps track of recently used packs is represented using two global variables: struct mru packed_git_mru_storage; struct mru *packed_git_mru = &packed_git_mru_storage; Callers never assign to the packed_git_mru pointer, though, so we can simplify by eliminating it and using &packed_git_mru_storage (renamed to &packed_git_mru) directly. This variable is only used by the packfile subsystem, making this a relatively uninvasive change (and any new unadapted callers would trigger a compile error). Noticed while moving these globals to the object_store struct. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Acked-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Kaartic Sivaraam	b3a8076e0d	help: change a message to be more precise When the user tries to use '--help' option on an aliased command information about the alias is printed as sshown below, $ git co --help `git co' is aliased to `checkout' This doesn't seem correct as the user has aliased only 'co' and not 'git co'. This might even be incorrect in cases in which the user has used an alias like 'tgit'. $ tgit co --help `git co' is aliased to `checkout' So, make the message more precise. Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Ross Kabus	c818e74332	commit-tree: do not complete line in -F input "git commit-tree -F <file>", unlike "cat <file> \| git commit-tree" (i.e. feeding the same contents from the standard input), added a missing final newline when the input ended in an incomplete line. Correct this inconsistency by leaving the incomplete line as-is, as erring on the side of not touching the input is preferrable and expected for a plumbing command like "commit-tree". Signed-off-by: Ross Kabus <rkabus@aerotech.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jeff King	1ab03a57e1	shortlog: skip format/parse roundtrip for internal traversal The original git-shortlog command parsed the output of git-log, and the logic went something like this: 1. Read stdin looking for "author" lines. 2. Parse the identity into its name/email bits. 3. Apply mailmap to the name/email. 4. Reformat the identity into a single buffer that is our "key" for grouping entries (either a name by default, or "name <email>" if --email was given). The first part happens in read_from_stdin(), and the other three steps are part of insert_one_record(). When we do an internal traversal, we just swap out the stdin read in step 1 for reading the commit objects ourselves. Prior to `2db6b83d18` (shortlog: replace hand-parsing of author with pretty-printer, 2016-01-18), that made sense; we still had to parse the ident in the commit message. But after that commit, we use pretty.c's "%an <%ae>" to get the author ident (for simplicity). Which means that the pretty printer is doing a parse/format under the hood, and then we parse the result, apply the mailmap, and format the result again. Instead, we can just ask pretty.c to do all of those steps for us (including the mailmap via "%aN <%aE>", and not formatting the address when --email is missing). And then we can push steps 2-4 into read_from_stdin(). This speeds up "git shortlog -ns" on linux.git by about 3%, and eliminates a leak in insert_one_record() of the namemailbuf strbuf. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jeff King	0e5bba53af	add UNLEAK annotation for reducing leak false positives It's a common pattern in git commands to allocate some memory that should last for the lifetime of the program and then not bother to free it, relying on the OS to throw it away. This keeps the code simple, and it's fast (we don't waste time traversing structures or calling free at the end of the program). But it also triggers warnings from memory-leak checkers like valgrind or LSAN. They know that the memory was still allocated at program exit, but they don't know _when_ the leaked memory stopped being useful. If it was early in the program, then it's probably a real and important leak. But if it was used right up until program exit, it's not an interesting leak and we'd like to suppress it so that we can see the real leaks. This patch introduces an UNLEAK() macro that lets us do so. To understand its design, let's first look at some of the alternatives. Unfortunately the suppression systems offered by leak-checking tools don't quite do what we want. A leak-checker basically knows two things: 1. Which blocks were allocated via malloc, and the callstack during the allocation. 2. Which blocks were left un-freed at the end of the program (and which are unreachable, but more on that later). Their suppressions work by mentioning the function or callstack of a particular allocation, and marking it as OK to leak. So imagine you have code like this: int cmd_foo(...) { /* this allocates some memory / char p = some_function(); printf("%s", p); return 0; } You can say "ignore allocations from some_function(), they're not leaks". But that's not right. That function may be called elsewhere, too, and we would potentially want to know about those leaks. So you can say "ignore the callstack when main calls some_function". That works, but your annotations are brittle. In this case it's only two functions, but you can imagine that the actual allocation is much deeper. If any of the intermediate code changes, you have to update the suppression. What we _really_ want to say is that "the value assigned to p at the end of the function is not a real leak". But leak-checkers can't understand that; they don't know about "p" in the first place. However, we can do something a little bit tricky if we make some assumptions about how leak-checkers work. They generally don't just report all un-freed blocks. That would report even globals which are still accessible when the leak-check is run. Instead they take some set of memory (like BSS) as a root and mark it as "reachable". Then they scan the reachable blocks for anything that looks like a pointer to a malloc'd block, and consider that block reachable. And then they scan those blocks, and so on, transitively marking anything reachable from a global as "not leaked" (or at least leaked in a different category). So we can mark the value of "p" as reachable by putting it into a variable with program lifetime. One way to do that is to just mark "p" as static. But that actually affects the run-time behavior if the function is called twice (you aren't likely to call main() twice, but some of our cmd_() functions are called from other commands). Instead, we can trick the leak-checker by putting the value into _any_ reachable bytes. This patch keeps a global linked-list of bytes copied from "unleaked" variables. That list is reachable even at program exit, which confers recursive reachability on whatever values we unleak. In other words, you can do: int cmd_foo(...) { char p = some_function(); printf("%s", p); UNLEAK(p); return 0; } to annotate "p" and suppress the leak report. But wait, couldn't we just say "free(p)"? In this toy example, yes. But UNLEAK()'s byte-copying strategy has several advantages over actually freeing the memory: 1. It's recursive across structures. In many cases our "p" is not just a pointer, but a complex struct whose fields may have been allocated by a sub-function. And in some cases (e.g., dir_struct) we don't even have a function which knows how to free all of the struct members. By marking the struct itself as reachable, that confers reachability on any pointers it contains (including those found in embedded structs, or reachable by walking heap blocks recursively. 2. It works on cases where we're not sure if the value is allocated or not. For example: char p = argc > 1 ? argv[1] : some_function(); It's safe to use UNLEAK(p) here, because it's not freeing any memory. In the case that we're pointing to argv here, the reachability checker will just ignore our bytes. 3. Likewise, it works even if the variable has _already_ been freed. We're just copying the pointer bytes. If the block has been freed, the leak-checker will skip over those bytes as uninteresting. 4. Because it's not actually freeing memory, you can UNLEAK() before we are finished accessing the variable. This is helpful in cases like this: char p = some_function(); return another_function(p); Writing this with free() requires: int ret; char p = some_function(); ret = another_function(p); free(p); return ret; But with unleak we can just write: char p = some_function(); UNLEAK(p); return another_function(p); This patch adds the UNLEAK() macro and enables it automatically when Git is compiled with SANITIZE=leak. In normal builds it's a noop, so we pay no runtime cost. It also adds some UNLEAK() annotations to show off how the feature works. On top of other recent leak fixes, these are enough to get t0000 and t0001 to pass when compiled with LSAN. Note the case in commit.c which actually converts a strbuf_release() into an UNLEAK. This code was already non-leaky, but the free didn't do anything useful, since we're exiting. Converting it to an annotation means that non-leak-checking builds pay no runtime cost. The cost is minimal enough that it's probably not worth going on a crusade to convert these kinds of frees to UNLEAKS. I did it here for consistency with the "sb" leak (though it would have been equally correct to go the other way, and turn them both into strbuf_release() calls). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Stefan Beller	f8b863598c	builtin/merge: honor commit-msg hook for merges Similar to `65969d43d1` (merge: honor prepare-commit-msg hook, 2011-02-14) merge should also honor the commit-msg hook: When a merge is stopped due to conflicts or --no-commit, the subsequent commit calls the commit-msg hook. However, it is not called after a clean merge. Fix this inconsistency by invoking the hook after clean merges as well. This change is motivated by Gerrit's commit-msg hook to install a ChangeId trailer into the commit message. Without such a ChangeId, Gerrit refuses to accept any commit by default, such that the inconsistency of (not) running the commit-msg hook between commit and merge leads to confusion and might block people from getting their work done. As the githooks man page is very vocal about the possibility of skipping the commit-msg hook via the --no-verify option, implement the option in merge, too. 'git merge --continue' is currently implemented as calling cmd_commit with no further arguments. This works for most other merge related options, such as demonstrated via the --allow-unrelated-histories flag in the test. The --no-verify option however is not remembered across invocations of git-merge. Originally the author assumed an alternative in which the 'git merge --continue' command accepts the --no-verify flag, but that opens up the discussion which flags are allows to the continued merge command and which must be given in the first invocation. Signed-off-by: Stefan Beller <sbeller@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago

1 2 3 4 5 ...

6137 Commits (58ebd936cc92441ec672b96e98ad12b34b3cb4a8)