summaryrefslogtreecommitdiff
AgeCommit message (Collapse)Author
2020-02-24packed_object_info(): use object_id internally for delta baseJeff King
The previous commit changed the public interface of packed_object_info() to return a struct object_id rather than a bare hash. That enables us to convert our internal helper, as well. We can use nth_packed_object_id() directly for OFS_DELTA, but we'll still have to use oidread() to pull the hash for a REF_DELTA out of the packfile. There should be no additional cost, since we're copying directly into the object_id the caller provided us (just as we did before; it's just happening now via nth_packed_object_id()). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24packed_object_info(): use object_id for returning delta baseJeff King
If a caller sets the object_info.delta_base_sha1 to a non-NULL pointer, we'll write the oid of the object's delta base to it. But we can increase our type safety by switching this to a real object_id struct. All of our callers are just pointing into the hash member of an object_id anyway, so there's no inconvenience. Note that we do still keep it as a pointer-to-struct, because the NULL sentinel value tells us whether the caller is even interested in the information. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24pack-check: push oid lookup into loopJeff King
When we're checking a pack with fsck or verify-pack, we first sort the idx entries by offset, since accessing them in pack order is more efficient. To do so, we loop over them and fill in an array of structs with the offset, object_id, and index position of each, sort the result, and only then do we iterate over the sorted array and process each entry. In order to avoid the memory cost of storing the hash of each object, we just store a pointer into the copy in the mmap'd pack index file. To keep that property even as the rest of the code converted to "struct object_id", commit 9fd750461b (Convert the verify_pack callback to struct object_id, 2017-05-06) introduced a union in order to type-pun the pointer-to-hash into an object_id struct. But we can make this even simpler by observing that the sort operation doesn't need the object id at all! We only need them one at a time while we actually process each entry. So we can just omit the oid from the struct entirely and load it on the fly into a local variable in the second loop. This gets rid of the type-punning, and lets us directly use the more type-safe nth_packed_object_id(), simplifying the code. And as a bonus, it saves 8 bytes of memory per object. Note that this does mean we'll do the offset lookup for each object before the oid lookup. The oid lookup has more safety checks in it (e.g., for looking past p->num_objects) which in theory protected the offset lookup. But since violating those checks was already a BUG() condition (as described in the previous commit), it's not worth worrying about. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24pack-check: convert "internal error" die to a BUG()Jeff King
If we fail to load the oid from the index of a packfile, we'll die() with an "internal error". But this should never happen: we'd fail here only if the idx needed to be lazily opened (but we've already opened it) or if we asked for an out-of-range index (but we're iterating using the same count that we'd check the range against). A corrupted index might have a bogus count (e.g., too large for its size), but we'd have complained and aborted already when opening the index initially. While we're here, we can add a few details so that if this bug ever _does_ trigger, we'll have a bit more information. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24pack-bitmap: use object_id when loading on-disk bitmapsJeff King
A pack bitmap file contains the index position of the commit for each bitmap, which we then translate into an object id via nth_packed_object_sha1(). In preparation for that function going away, we can switch to the more type-safe nth_packed_object_id(). Note that even though the result ends up in an object_id this does incur an extra copy of the hash (into our temporary object_id, and then into the final malloc'd stored_bitmap struct). This shouldn't make any measurable difference. If it did, we could avoid this copy _and_ the copy of the rest of the items by allocating the stored_bitmap struct beforehand and reading directly into it from the bitmap file. Or better still, if this is a bottleneck, we could introduce an on-disk index to the bitmap file so we don't have to read every single entry to use just one of them. So it's not worth worrying about micro-optimizing out this one hash copy. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24pack-objects: use object_id struct in pack-reuse codeJeff King
When the pack-reuse code is dumping an OFS_DELTA entry to a client that doesn't support it, we re-write it as a REF_DELTA. To do so, we use nth_packed_object_sha1() to get the oid, but that function is soon going away in favor of the more type-safe nth_packed_object_id(). Let's switch now in preparation. Note that this does incur an extra hash copy (from the pack idx mmap to the object_id and then to the output, rather than straight from mmap to the output). But this is not worth worrying about. It's probably not measurable even when it triggers, and this is fallback code that we expect to trigger very rarely (since everybody supports OFS_DELTA these days anyway). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24pack-objects: convert oe_set_delta_ext() to use object_idJeff King
We already store an object_id internally, and now our sole caller also has one. Let's stop passing around the internal hash array, which adds a bit of type safety. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24pack-objects: read delta base oid into object_id structJeff King
When we're considering reusing an on-disk delta, we get the oid of the base as a pointer to unsigned char bytes of the hash, either into the packfile itself (for REF_DELTA) or into the pack idx (using the revindex to convert the offset into an index entry). Instead, we'd prefer to use a more type-safe object_id as much as possible. We can get the pack idx using nth_packed_object_id() instead. For the packfile bytes, we can copy them out using oidread(). This doesn't even incur an extra copy overall, since the next thing we'd always do with that pointer is pass it to can_reuse_delta(), which needs an object_id anyway (and called oidread() itself). So this patch also converts that function to take the object_id directly. Note that we did previously use NULL as a sentinel value when the object isn't a delta. We could probably get away with using the null oid for this, but instead we'll use an explicit boolean flag, which should make things more obvious for people reading the code later. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24nth_packed_object_oid(): use customary integer returnJeff King
Our nth_packed_object_sha1() function returns NULL for error. So when we wrapped it with nth_packed_object_oid(), we kept the same semantics. But it's a bit funny, because the caller actually passes in an out parameter, and the pointer we return is just that same struct they passed to us (or NULL). It's not too terrible, but it does make the interface a little non-idiomatic. Let's switch to our usual "0 for success, negative for error" return value. Most callers either don't check it, or are trivially converted. The one that requires the biggest change is actually improved, as we can ditch an extra aliased pointer variable. Since we are changing the interface in a subtle way that the compiler wouldn't catch, let's also change the name to catch any topics in flight. We can drop the 'o' and make it nth_packed_object_id(). That's slightly shorter, but also less redundant since the 'o' stands for "object" already. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24worktree: drop unused code from get_main_worktree()Eric Sunshine
This code has been unused since fa099d2322 (worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe(), 2017-04-24), so drop it. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24blame: provide type of fingerprints pointerRené Scharfe
The fingerprints member of struct blame_origin is a void pointer that is only ever used to reference objects of type struct fingerprint. Declare its type to allow the compiler to do type checks. We can keep its type opaque in blame.h, though -- only functions in blame.c need to know the actual definition of struct fingerprint. Signed-off-by: René Scharfe <l.s.r@web.de> Reviewed-by: Barret Rhoden <brho@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24rebase: refuse to switch to branch already checked out elsewhereEric Sunshine
The invocation "git rebase <upstream> <branch>" switches to <branch> before performing the rebase operation. However, unlike git-switch, git-checkout, and git-worktree which all refuse to switch to a branch that is already checked out in some other worktree, git-rebase switches to <branch> unconditionally. Curb this careless behavior by making git-rebase also refuse to switch to a branch checked out elsewhere. Reported-by: Mike Hommey <mh@glandium.org> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24t3400: make test clean up after itselfEric Sunshine
This test intentionally creates a file which causes rebase to fail, thus it is important that this file be deleted before subsequent tests are run which are not expecting such a failure. In the past, the common way to ensure cleanup (regardless of whether the test succeeded or failed) was either for the next test to perform the previous test's cleanup as its first step or to do the cleanup at global scope outside of any tests. With the introduction of 'test_when_finished', however, tests can be responsible for their own cleanup. Therefore, update this test to clean up after itself. A bit of history: This 'rm' invocation was moved from within the body of the following test to global scope by bffd750adf (rebase: improve error message when upstream argument is missing, 2010-05-31), which postdates, by about a month, introduction of 'test_when_finished' in 3bf7886705 (test-lib: Let tests specify commands to be run at end of test, 2010-05-02). Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24t: drop debug `cat` callsMartin Ågren
We `cat` files, but don't inspect or grab the contents in any way. Unlike in an earlier commit, there is no reason to suspect that these files could be missing, so `cat`-ing them is just wasted effort. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24t9810: drop debug `cat` callMartin Ågren
We `cat` kwdelfile.c, but don't inspect or grab the contents in any way. This looks like a remnant from a debug session. Similar to the previous commit, one could argue that `cat`-ing the file verifies that it didn't disappear somehow. But because the very next thing we do after `cat`-ing the file is to `grep` in it, we can safely drop the call to `cat`. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24t4117: check for files using `test_path_is_file`Martin Ågren
We `cat` files, but don't inspect or grab the contents in any way. These `cat` calls look like remnants from a debug session, so it's tempting to get rid of them. But they do actually verify that the files exist, which might not necessarily be the case for some failure modes of `git apply --reject`. Let's not lose that. Convert the `cat` calls to use `test_path_is_file` instead. This is of course still a minor change since we no longer verify that the files can be opened for reading, but that is not something we usually worry about. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24receive.denyCurrentBranch: respect all worktreesHariom Verma
The receive.denyCurrentBranch config option controls what happens if you push to a branch that is checked out into a non-bare repository. By default, it rejects it. It can be disabled via `ignore` or `warn`. Another yet trickier option is `updateInstead`. However, this setting was forgotten when the git worktree command was introduced: only the main worktree's current branch is respected. With this change, all worktrees are respected. That change also leads to revealing another bug, i.e. `receive.denyCurrentBranch = true` was ignored when pushing into a non-bare repository's unborn current branch using ref namespaces. As `is_ref_checked_out()` returns 0 which means `receive-pack` does not get into conditional statement to switch `deny_current_branch` accordingly (ignore, warn, refuse, unconfigured, updateInstead). receive.denyCurrentBranch uses the function `refs_resolve_ref_unsafe()` (called via `resolve_refdup()`) to resolve the symbolic ref HEAD, but that function fails when HEAD does not point at a valid commit. As we replace the call to `refs_resolve_ref_unsafe()` with `find_shared_symref()`, which has no problem finding the worktree for a given branch even if it is unborn yet, this bug is fixed at the same time: receive.denyCurrentBranch now also handles worktrees with unborn branches as intended even while using ref namespaces. Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24t5509: use a bare repository for test push targetHariom Verma
`receive.denyCurrentBranch` currently has a bug where it allows pushing into non-bare repository using namespaces as long as it does not have any commits. This would cause t5509 to fail once that bug is fixed because it pushes into an unborn current branch. In t5509, no operations are performed inside `pushee`, as it is only a target for `git push` and `git ls-remote` calls. Therefore it does not need to have a worktree. So, it is safe to change `pushee` to a bare repository. Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24get_main_worktree(): allow it to be called in the Git directoryHariom Verma
When called in the Git directory of a non-bare repository, this function would not return the directory of the main worktree, but of the Git directory instead. The reason: when the Git directory is the current working directory, the absolute path of the common directory will be reported with a trailing `/.git/.`, which the code of `get_main_worktree()` does not handle correctly. Let's fix this. Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Hariom Verma <hariom18599@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24use strpbrk(3) to search for characters from a given setRené Scharfe
We can check if certain characters are present in a string by calling strchr(3) on each of them, or we can pass them all to a single strpbrk(3) call. The latter is shorter, less repetitive and slightly more efficient, so let's do that instead. Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-24quote: use isalnum() to check for alphanumeric charactersRené Scharfe
isalnum(c) is equivalent to isalpha(c) || isdigit(c), so use the former instead. The result is shorter, simpler and slightly more efficient. Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-22partial-clone: avoid fetching when looking for objectsDerrick Stolee
When using partial clone, find_non_local_tags() in builtin/fetch.c checks each remote tag to see if its object also exists locally. There is no expectation that the object exist locally, but this function nevertheless triggers a lazy fetch if the object does not exist. This can be extremely expensive when asking for a commit, as we are completely removed from the context of the non-existent object and thus supply no "haves" in the request. 6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05) removed a global variable that prevented these fetches in favor of a bitflag. However, some object existence checks were not updated to use this flag. Update find_non_local_tags() to use OBJECT_INFO_SKIP_FETCH_OBJECT in addition to OBJECT_INFO_QUICK. The _QUICK option only prevents repreparing the pack-file structures. We need to be extremely careful about supplying _SKIP_FETCH_OBJECT when we expect an object to not exist due to updated refs. This resolves a broken test in t5616-partial-clone.sh. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-22partial-clone: demonstrate bugs in partial fetchDerrick Stolee
While testing partial clone, I noticed some odd behavior. I was testing a way of running 'git init', followed by manually configuring the remote for partial clone, and then running 'git fetch'. Astonishingly, I saw the 'git fetch' process start asking the server for multiple rounds of pack-file downloads! When tweaking the situation a little more, I discovered that I could cause the remote to hang up with an error. Add two tests that demonstrate these two issues. In the first test, we find that when fetching with blob filters from a repository that previously did not have any tags, the 'git fetch --tags origin' command fails because the server sends "multiple filter-specs cannot be combined". This only happens when using protocol v2. In the second test, we see that a 'git fetch origin' request with several ref updates results in multiple pack-file downloads. This must be due to Git trying to fault-in the objects pointed by the refs. What makes this matter particularly nasty is that this goes through the do_oid_object_info_extended() method, so there are no "haves" in the negotiation. This leads the remote to send every reachable commit and tree from each new ref, providing a quadratic amount of data transfer! This test is fixed if we revert 6462d5eb9a (fetch: remove fetch_if_missing=0, 2019-11-05), but that revert causes other test failures. The real fix will need more care. The tests are ordered in this way because if I swap the test order the tag test will succeed instead of fail. I believe this is because somehow we need the srv.bare repo to not have any tags when we clone, but then have tags in our next fetch. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-22run-command.h: fix mis-indented struct memberJeff King
An accidental conversion of a tab to 4 spaces snuck into 4c4066d95d (run-command: move doc to run-command.h, 2019-11-17), messing up the alignment when you have the project-recommended 8-width tabstops. Let's revert that line. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-20sparse-checkout: allow one-character directories in cone modeDerrick Stolee
In 9e6d3e64 (sparse-checkout: detect short patterns, 2020-01-24), a condition on the minimum length of a cone-mode pattern was introduced. However, this condition was off-by-one. If we have a directory with a single character, say "b", then the command git sparse-checkout set b will correctly add the pattern "/b/" to the sparse-checkout file. When this is interpeted in dir.c, the pattern is "/b" with the PATTERN_FLAG_MUSTBEDIR flag. This string has length two, which satisfies our inclusive inequality (<= 2). The reason for this inequality is that we will start to read the pattern string character-by-character using three char pointers: prev, cur, next. In particular, next is set to the current pattern plus two. The mistake was that next will still be a valid pointer when the pattern length is two, since the string is null-terminated. Make this inequality strict so these patterns work. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-20credential: allow wildcard patterns when matching configbrian m. carlson
In some cases, a user will want to use a specific credential helper for a wildcard pattern, such as https://*.corp.example.com. We have code that handles this already with the urlmatch code, so let's use that instead of our custom code. Since the urlmatch code is a superset of our current matching in terms of capabilities, there shouldn't be any cases of things that matched previously that don't match now. However, in addition to wildcard matching, we now use partial path matching, which can cause slightly different behavior in the case that a helper applies to the prefix (considering path components) of the remote URL. While different, this is probably the behavior people were wanting anyway. Since we're using the urlmatch code, we need to encode the components we've gotten into a URL to match, so add a function to percent-encode data and format the URL with it. We now also no longer need to the custom code to match URLs, so let's remove it. Additionally, the urlmatch code always looks for the best match, whereas we want all matches for credential helpers to preserve existing behavior. Let's add an optional field, select_fn, that lets us control which items we want (in this case, all of them) and default it to the best-match code that already exists for other users. Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-20credential: use the last matching username in the configbrian m. carlson
Everywhere else in the codebase, we use the rule that the last matching configuration option is the one that takes effect. This is helpful because it allows more specific configuration settings (e.g., per-repo configuration) to override less specific settings (e.g., per-user configuration). However, in the credential code, we didn't honor this setting, and instead picked the first setting we had, and stuck with it. This was likely to ensure we picked the value from the URL, which we want to honor over the configuration. It's possible to do both, though, so let's check if the value is the one we've gotten over our protocol connection, which if present will have come from the URL, and keep it if so. Otherwise, let's overwrite the value with the latest version we've got from the configuration, so we keep the last configuration value. Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-20t0300: add tests for some additional casesbrian m. carlson
There are some tricky cases in our credential helpers that we don't have test cases for. To help prevent regressions, let's add some for these cases: * If there are multiple configured credential helpers, one without a path and one with a path, we want to invoke both of them. * If there are percent-encoded values in the URL, we handle them properly. * And finally, if there is a username in the remote URL, we want to honor that over what the configuration tells us. Finally, there's an additional case that we'd like to test for as well, but that currently fails. In all other situations in our configuration, we pick the last configuration setting that's provided. However, we fail to do that for credential.username, where we pick the first setting instead. Let's add a failing test that we have the consistent behavior here, since that's the documented, expected behavior. Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-20t1300: add test for urlmatch with multiple wildcardsbrian m. carlson
Our urlmatch code handles multiple wildcards, but we don't currently have a test that checks this code path. Add a test that we handle this case correctly to avoid any regressions. Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-20mailmap: add an additional email address for brian m. carlsonbrian m. carlson
To more accurately track the provenance of contributions, brian uses a work email address for commits created at work. Add this email address to .mailmap so that contributions are properly attributed to the same person. Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19merge-recursive: fix the refresh logic in update_file_flagsElijah Newren
If we need to delete a higher stage entry in the index to place the file at stage 0, then we'll lose that file's stat information. In such situations we may still be able to detect that the file on disk is the version we want (as noted by our comment in the code: /* do not overwrite file if already present */ ), but we do still need to update the mtime since we are creating a new cache_entry for that file. Update the logic used to determine whether we refresh a file's mtime. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19t3433: new rebase testcase documenting a stat-dirty-like failureElijah Newren
A user discovered a case where they had a stack of 20 simple commits to rebase, and the rebase would succeed in picking the first commit and then error out with a pair of "Could not execute the todo command" and "Your local changes to the following files would be overwritten by merge" messages. Their steps actually made use of the -i flag, but I switched it over to -m to make it simpler to trigger the bug. With that flag, it bisects back to commit 68aa495b590d (rebase: implement --merge via the interactive machinery, 2018-12-11), but that's misleading. If you change the -m flag to --keep-empty, then the problem persists and will bisect back to 356ee4659bb5 (sequencer: try to commit without forking 'git commit', 2017-11-24) After playing with the testcase for a bit, I discovered that added --exec "sleep 1" to the command line makes the rebase succeed, making me suspect there is some kind of discard and reloading of caches that lead us to believe that something is stat dirty, but I didn't succeed in digging any further than that. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect: libify `bisect_next_all`Pranit Bauva
Since we want to get rid of git-bisect.sh, it would be necessary to convert those exit() calls to return statements so that errors can be reported. Emulate try catch in C by converting `exit(<positive-value>)` to `return <negative-value>`. Follow POSIX conventions to return <negative-value> to indicate error. All the functions calling `bisect_next_all()` are already able to handle return values from it. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect: libify `handle_bad_merge_base` and its dependentsPranit Bauva
Since we want to get rid of git-bisect.sh, it would be necessary to convert those exit() calls to return statements so that errors can be reported. Emulate try catch in C by converting `exit(<positive-value>)` to `return <negative-value>`. Follow POSIX conventions to return <negative-value> to indicate error. Update all callers to handle the error returns. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect: libify `check_good_are_ancestors_of_bad` and its dependentsPranit Bauva
Since we want to get rid of git-bisect.sh, it would be necessary to convert those exit() calls to return statements so that errors can be reported. Emulate try catch in C by converting `exit(<positive-value>)` to `return <negative-value>`. Follow POSIX conventions to return <negative-value> to indicate error. Code that turns BISECT_INTERNAL_SUCCESS_MERGE_BASE (-11) to BISECT_OK (0) from `check_good_are_ancestors_of_bad()` has been moved to `cmd_bisect__helper()`. Update all callers to handle the error returns. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect: libify `check_merge_bases` and its dependentsPranit Bauva
Since we want to get rid of git-bisect.sh, it would be necessary to convert those exit() calls to return statements so that errors can be reported. Emulate try catch in C by converting `exit(<positive-value>)` to `return <negative-value>`. Follow POSIX conventions to return <negative-value> to indicate error. In `check_merge_bases()` there is an early success special case, so we have introduced special error code BISECT_INTERNAL_SUCCESS_MERGE_BASE (-11) which indicates early success. This BISECT_INTERNAL_SUCCESS_MERGE_BASE is converted back to BISECT_OK (0) in `check_good_are_ancestors_of_bad()`. Update all callers to handle the error returns. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect: libify `bisect_checkout`Pranit Bauva
Since we want to get rid of git-bisect.sh, it would be necessary to convert those exit() calls to return statements so that errors can be reported. Emulate try catch in C by converting `exit(<positive-value>)` to `return <negative-value>`. Follow POSIX conventions to return <negative-value> to indicate error. Turn `exit()` to `return` calls in `bisect_checkout()`. Changes related to return values have no bad side effects on the code that calls `bisect_checkout()`. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its ↵Pranit Bauva
dependents Since we want to get rid of git-bisect.sh, it would be necessary to convert those exit() calls to return statements so that errors can be reported. Emulate try catch in C by converting `exit(<positive-value>)` to `return <negative-value>`. Follow POSIX conventions to return <negative-value> to indicate error. Update all callers to handle the error returns. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect--helper: return error codes from `cmd_bisect__helper()`Pranit Bauva
Since we want to get rid of git-bisect.sh, it would be necessary to convert bisect.c exit() calls to return statements so that errors can be reported. Let's prepare for that by making it possible to return different error codes than just 0 or 1. Different error codes might enable a bisecting script calling the bisect command that uses this function to do different things depending on the exit status of the bisect command. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect: add enum to represent bisect returning codesMiriam Rubio
Since we want to get rid of git-bisect.sh, it would be necessary to convert those exit() calls to return statements so that errors can be reported. Create an enum called `bisect_error` with the bisecting return codes to use in `bisect.c` libification process. Change bisect_next_all() to make it return this enum. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect--helper: introduce new `decide_next()` functionTanushree Tumane
Let's refactor code from bisect_next_check() into a new decide_next() helper function. This removes some goto statements and makes the code simpler, clearer and easier to understand. While at it `bad_ref` and `good_glob` are not const any more to void casting them inside `free()`. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect: use the standard 'if (!var)' way to check for 0Miriam Rubio
Instead of using 'var == 0' in an if condition, let's use '!var' and make 'bisect.c' more consistent with the rest of the code. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect--helper: change `retval` to `res`Tanushree Tumane
Let's rename variable retval to res, so that variable names in bisect--helper.c are more consistent. After this change, there are 110 occurrences of res in the file and zero of retval, while there were 26 instances of retval before. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-19bisect--helper: convert `vocab_*` char pointers to char arraysMiriam Rubio
Instead of using a pointer that points at a constant string, just give name directly to the constant string; this way, we do not have to allocate a pointer variable in addition to the string we want to use. Let's convert `vocab_bad` and `vocab_good` char pointers to char arrays. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Miriam Rubio <mirucam@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-18check-ignore: fix documentation and implementation to matchElijah Newren
check-ignore has two different modes, and neither of these modes has an implementation that matches the documentation. These modes differ in whether they just print paths or whether they also print the final pattern matched by the path. The fix is different for both modes, so I'll discuss both separately. === First (default) mode === The first mode is documented as: For each pathname given via the command-line or from a file via --stdin, check whether the file is excluded by .gitignore (or other input files to the exclude mechanism) and output the path if it is excluded. However, it fails to do this because it did not account for negated patterns. Commands other than check-ignore verify exclusion rules via calling ... -> treat_one_path() -> is_excluded() -> last_matching_pattern() while check-ignore has a call path of the form: ... -> check_ignore() -> last_matching_pattern() The fact that the latter does not include the call to is_excluded() means that it is susceptible to to messing up negated patterns (since that is the only significant thing is_excluded() adds over last_matching_pattern()). Unfortunately, we can't make it just call is_excluded(), because the same codepath is used by the verbose mode which needs to know the matched pattern in question. This brings us to... === Second (verbose) mode === The second mode, known as verbose mode, references the first in the documentation and says: Also output details about the matching pattern (if any) for each given pathname. For precedence rules within and between exclude sources, see gitignore(5). The "Also" means it will print patterns that match the exclude rules as noted for the first mode, and also print which pattern matches. Unless more information is printed than just pathname and pattern (which is not done), this definition is somewhat ill-defined and perhaps even self-contradictory for negated patterns: A path which matches a negated exclude pattern is NOT excluded and thus shouldn't be printed by the former logic, while it certainly does match one of the explicit patterns and thus should be printed by the latter logic. === Resolution == Since the second mode exists to find out which pattern matches given paths, and showing the user a pattern that begins with a '!' is sufficient for them to figure out whether the pattern is excluded, the existing behavior is desirable -- we just need to update the documentation to match the implementation (i.e. it is about printing which pattern is matched by paths, not about showing which paths are excluded). For the first or default mode, users just want to know whether a pattern is excluded. As such, the existing documentation is desirable; change the implementation to match the documented behavior. Finally, also adjust a few tests in t0008 that were caught up by this discrepancy in how negated paths were handled. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-18doc-diff: use single-colon rule in rendering MakefileJeff King
When rendering the troff manpages to text via "man", we create an ad-hoc Makefile and feed it to "make". The purpose here is two-fold: - reuse results from a prior interrupted render of the same tree - use make's -j option to build in parallel But the second part doesn't seem to work (at least with my version of GNU make, 4.2.1). It just runs one render at a time. We use a double-colon "all" rule for each file, like: all:: foo foo: ...actual render recipe... all:: bar bar: ...actual render recipe... ...and so on... And it's this double-colon that seems to inhibit the parallelism. We can just switch to a regular single-colon rule. Even though we do have multiple rules for "all" here, we don't have any recipe to execute for "all" (we only care about triggering its dependencies), so the distinction is irrelevant. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-18doc/config/push: use longer "--" line for preformatted exampleJeff King
The example for the push.pushOption config tries to create a preformatted section, but uses only two dashes in its "--" line. In AsciiDoc this is an "open block", with no type; the lines end up jumbled because they're formatted as paragraphs. We need four or more dashes to make it a "listing block" that will respect the linebreaks. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-18rev-list --count: comment on the use of count_right++Junio C Hamano
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-17The sixth batch for 2.26Junio C Hamano
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-17Merge branch 'es/outside-repo-errmsg-hints'Junio C Hamano
Error message clarification. * es/outside-repo-errmsg-hints: prefix_path: show gitdir when arg is outside repo