summaryrefslogtreecommitdiff
path: root/builtin
AgeCommit message (Collapse)AuthorFilesLines
2020-12-03builtin/clone.c: don't ignore transport_fetch_refs() errorsLibravatar Taylor Blau1-4/+11
If 'git clone' couldn't execute 'transport_fetch_refs()' (e.g., because of an error on the remote's side in 'git upload-pack'), then it will silently ignore it. Even though this has been the case at least since clone was ported to C (way back in 8434c2f1af (Build in clone, 2008-04-27)), 'git fetch' doesn't ignore these and reports any failures it sees. That suggests that ignoring the return value in 'git clone' is simply an oversight that should be corrected. That's exactly what this patch does. (Noticing and fixing this is no coincidence, we'll want it in the next patch in order to demonstrate a regression in 'git upload-pack' via a 'git clone'.) There's no additional logging here, but that matches how 'git fetch' handles the same case. An assumption there is that whichever part of transport_fetch_refs() fails will complain loudly, so any additional logging here is redundant. Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-12-03Merge branch 'tb/repack-simplify'Libravatar Junio C Hamano1-99/+54
Simplify the logic to deal with a repack operation that ended up creating the same packfile. * tb/repack-simplify: builtin/repack.c: don't move existing packs out of the way builtin/repack.c: keep track of what pack-objects wrote repack: make "exts" array available outside cmd_repack()
2020-12-03Merge branch 'pb/pull-rebase-recurse-submodules'Libravatar Junio C Hamano1-12/+36
"git pull --rebase --recurse-submodules" checked for local changes in a wrong range and failed to run correctly when it should. * pb/pull-rebase-recurse-submodules: pull: check for local submodule modifications with the right range t5572: describe '--rebase' tests a little more t5572: add notes on a peculiar test pull --rebase: compute rebase arguments in separate function
2020-11-30Merge branch 'sa/credential-store-timeout'Libravatar Junio C Hamano1-2/+6
Multiple "credential-store" backends can race to lock the same file, causing everybody else but one to fail---reattempt locking with some timeout to reduce the rate of the failure. * sa/credential-store-timeout: crendential-store: use timeout when locking file
2020-11-30Merge branch 'km/stash-error-message-fix'Libravatar Junio C Hamano1-1/+1
Error message fix. * km/stash-error-message-fix: stash: add missing space to an error message
2020-11-30Merge branch 'mt/worktree-error-message-fix'Libravatar Junio C Hamano1-2/+2
Fix formulation of an error message with two placeholders in "git worktree add" subcommand. * mt/worktree-error-message-fix: worktree: fix order of arguments in error message
2020-11-30Merge branch 'ab/gc-keep-base-option'Libravatar Junio C Hamano1-4/+4
Fix an option name in "gc" documentation. * ab/gc-keep-base-option: gc: rename keep_base_pack variable for --keep-largest-pack gc docs: change --keep-base-pack to --keep-largest-pack
2020-11-30Merge branch 'js/pull-rebase-use-advise'Libravatar Junio C Hamano1-12/+12
UI improvement. * js/pull-rebase-use-advise: pull: colorize the hint about setting `pull.rebase`
2020-11-25Merge branch 'rs/gc-sort-func-cast-fix'Libravatar Junio C Hamano1-4/+2
Fix broken sorting of maintenance tasks. * rs/gc-sort-func-cast-fix: gc: fix cast in compare_tasks_by_selection()
2020-11-25Merge branch 'jk/4gb-idx'Libravatar Junio C Hamano2-4/+4
The code was not prepared to deal with pack .idx file that is larger than 4GB. * jk/4gb-idx: packfile: detect overflow in .idx file size checks block-sha1: take a size_t length parameter fsck: correctly compute checksums on idx files larger than 4GB use size_t to store pack .idx byte offsets compute pack .idx byte offsets using size_t
2020-11-25Merge branch 'jx/t5411-flake-fix'Libravatar Junio C Hamano1-25/+69
The exchange between receive-pack and proc-receive hook did not carefully check for errors. * jx/t5411-flake-fix: receive-pack: use default version 0 for proc-receive receive-pack: gently write messages to proc-receive t5411: new helper filter_out_user_friendly_and_stable_output
2020-11-25crendential-store: use timeout when locking fileLibravatar Simão Afonso1-2/+6
When holding the lock for rewriting the credential file, use a timeout to avoid race conditions when the credentials file needs to be updated in parallel. An example would be doing `fetch --all` on a repository with several remotes that need credentials, using parallel fetching. The timeout can be configured using "credentialStore.lockTimeoutMS", defaulting to 1 second. Signed-off-by: Simão Afonso <simao.afonso@powertools-tech.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-24stash: add missing space to an error messageLibravatar Kyle Meyer1-1/+1
Restore a space that was lost in 8a0fc8d19d (stash: convert apply to builtin, 2019-02-25). Signed-off-by: Kyle Meyer <kyle@kyleam.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-21Merge branch 'en/strmap'Libravatar Junio C Hamano3-65/+11
A specialization of hashmap that uses a string as key has been introduced. Hopefully it will see wider use over time. * en/strmap: shortlog: use strset from strmap.h Use new HASHMAP_INIT macro to simplify hashmap initialization strmap: take advantage of FLEXPTR_ALLOC_STR when relevant strmap: enable allocations to come from a mem_pool strmap: add a strset sub-type strmap: split create_entry() out of strmap_put() strmap: add functions facilitating use as a string->int map strmap: enable faster clearing and reusing of strmaps strmap: add more utility functions strmap: new utility functions hashmap: provide deallocation function names hashmap: introduce a new hashmap_partial_clear() hashmap: allow re-use after hashmap_free() hashmap: adjust spacing to fix argument alignment hashmap: add usage documentation explaining hashmap_free[_entries]()
2020-11-21Merge branch 'jk/rev-parse-end-of-options'Libravatar Junio C Hamano1-45/+55
"git rev-parse" learned the "--end-of-options" to help scripts to safely take a parameter that is supposed to be a revision, e.g. "git rev-parse --verify -q --end-of-options $rev". * jk/rev-parse-end-of-options: rev-parse: handle --end-of-options rev-parse: put all options under the "-" check rev-parse: don't accept options after dashdash
2020-11-21Merge branch 'jc/format-patch-name-max'Libravatar Junio C Hamano1-6/+14
The maximum length of output filenames "git format-patch" creates has become configurable (used to be capped at 64). * jc/format-patch-name-max: format-patch: make output filename configurable
2020-11-21worktree: fix order of arguments in error messageLibravatar Matheus Tavares1-2/+2
`git worktree add` (without --force) errors out when given a path that is already registered as a worktree and the path is missing on disk. But the `cmd` and `path` strings are switched on the error message. Let's fix that. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-21gc: rename keep_base_pack variable for --keep-largest-packLibravatar Ævar Arnfjörð Bjarmason1-4/+4
As noted in an earlier change the keep_base_pack variable name is a relic from an earlier on-list version of ae4e89e549 ("gc: add --keep-largest-pack option", 2018-04-15) before it was renamed to --keep-largest-pack. Let's change the variable name to avoid that confusion, it's easier to read the code if there's a 1=1 mapping between the variable name and option name. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-19pull: colorize the hint about setting `pull.rebase`Libravatar Johannes Schindelin1-12/+12
In d18c950a69f (pull: warn if the user didn't say whether to rebase or to merge, 2020-03-09), a new hint was introduced to encourage users to make a conscious decision about whether they want their pull to merge or to rebase by configuring the `pull.rebase` setting. This warning was clearly intended to advise users, but as pointed out in https://lore.kernel.org/git/87ima2rdsm.fsf%40evledraar.gmail.com, it uses `warning()` instead of `advise()`. One consequence is that the advice is not colorized in the same manner as other, similar messages. So let's use `advise()` instead. Pointed-out-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-18gc: fix cast in compare_tasks_by_selection()Libravatar René Scharfe1-4/+2
compare_tasks_by_selection() is used with QSORT and gets passed pointers to the elements of "static struct maintenance_task tasks[]". It casts the *addresses* of these passed pointers to element pointers, though, and thus effectively compares some unrelated values from the stack. Fix the casts to actually compare array elements. Detected by USan (make SANITIZE=undefined test). Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-18Merge branch 'pb/blame-funcname-range-userdiff'Libravatar Junio C Hamano2-7/+8
"git blame -L :funcname -- path" did not work well for a path for which a userdiff driver is defined. * pb/blame-funcname-range-userdiff: blame: simplify 'setup_blame_bloom_data' interface blame: simplify 'setup_scoreboard' interface blame: enable funcname blaming with userdiff driver line-log: mention both modes in 'blame' and 'log' short help doc: add more pointers to gitattributes(5) for userdiff blame-options.txt: also mention 'funcname' in '-L' description doc: line-range: improve formatting doc: log, gitk: move '-L' description to 'line-range-options.txt'
2020-11-18Merge branch 'en/merge-ort-api-null-impl'Libravatar Junio C Hamano3-4/+42
Preparation for a new merge strategy. * en/merge-ort-api-null-impl: merge,rebase,revert: select ort or recursive by config or environment fast-rebase: demonstrate merge-ort's API via new test-tool command merge-ort-wrappers: new convience wrappers to mimic the old merge API merge-ort: barebones API of new merge strategy with empty implementation
2020-11-18Merge branch 'ds/maintenance-part-3'Libravatar Junio C Hamano2-6/+333
Parts of "git maintenance" to ease writing crontab entries (and other scheduling system configuration) for it. * ds/maintenance-part-3: maintenance: add troubleshooting guide to docs maintenance: use 'incremental' strategy by default maintenance: create maintenance.strategy config maintenance: add start/stop subcommands maintenance: add [un]register subcommands for-each-repo: run subcommands on configured repos maintenance: add --schedule option and config maintenance: optionally skip --auto process
2020-11-18Merge branch 'pw/rebase-i-orig-head'Libravatar Junio C Hamano1-10/+10
"git rebase -i" did not store ORIG_HEAD correctly. * pw/rebase-i-orig-head: rebase -i: simplify get_revision_ranges() rebase -i: use struct object_id when writing state rebase -i: use struct object_id rather than looking up commit rebase -i: stop overwriting ORIG_HEAD buffer
2020-11-18Merge branch 'jk/format-patch-output'Libravatar Junio C Hamano1-15/+22
"git format-patch --output=there" did not work as expected and instead crashed. The option is now supported. * jk/format-patch-output: format-patch: support --output option format-patch: tie file-opening logic to output_directory format-patch: refactor output selection
2020-11-18Merge branch 'jc/line-log-takes-no-pathspec'Libravatar Junio C Hamano1-0/+3
"git log -L<range>:<path>" is documented to take no pathspec, but this was not enforced by the command line option parser, which has been corrected. * jc/line-log-takes-no-pathspec: log: diagnose -L used with pathspec as an error
2020-11-18Merge branch 'rs/empty-reflog-check-fix'Libravatar Junio C Hamano1-14/+13
The code to see if "git stash drop" can safely remove refs/stash has been made more carerful. * rs/empty-reflog-check-fix: stash: simplify reflog emptiness check
2020-11-17builtin/repack.c: don't move existing packs out of the wayLibravatar Taylor Blau1-89/+14
When 'git repack' creates a pack with the same name as any existing pack, it moves the existing one to 'old-pack-xxx.{pack,idx,...}' and then renames the new one into place. Eventually, it would be nice to have 'git repack' allow for writing a multi-pack index at the critical time (after the new packs have been written / moved into place, but before the old ones have been deleted). Guessing that this option might be called '--write-midx', this makes the following situation (where repacks are issued back-to-back without any new objects) impossible: $ git repack -adb $ git repack -adb --write-midx In the second repack, the existing packs are overwritten verbatim with the same rename-to-old sequence. At that point, the current MIDX is invalidated, since it refers to now-missing packs. So that code wants to be run after the MIDX is re-written. But (prior to this patch) the new MIDX can't be written until the new packs are moved into place. So, we have a circular dependency. This is all hypothetical, since no code currently exists to write a MIDX safely during a 'git repack' (the 'GIT_TEST_MULTI_PACK_INDEX' does so unsafely). Putting hypothetical aside, though: why do we need to rename existing packs to be prefixed with 'old-' anyway? This behavior dates all the way back to 2ad47d6 (git-repack: Be careful when updating the same pack as an existing one., 2006-06-25). 2ad47d6 is mainly concerned about a case where a newly written pack would have a different structure than its index. This used to be possible when the pack name was a hash of the set of objects. Under this naming scheme, two packs that store the same set of objects could differ in delta selection, object positioning, or both. If this happened, then any such packs would be unreadable in the instant between copying the new pack and new index (i.e., either the index or pack will be stale depending on the order that they were copied). But since 1190a1a (pack-objects: name pack files after trailer hash, 2013-12-05), this is no longer possible, since pack files are named not after their logical contents (i.e., the set of objects), but by the actual checksum of their contents. So, this old- behavior can safely go, which allows us to avoid our circular dependency above. In addition to avoiding the circular dependency, this patch also makes 'git repack' a lot simpler, since we don't have to deal with failures encountered when renaming existing packs to be prefixed with 'old-'. This patch is mostly limited to removing code paths that deal with the 'old' prefixing, with the exception of files that include the pack's name in their own filename, like .idx, .bitmap, and related files. The exception is that we want to continue to trust what pack-objects wrote. That is, it is not the case that we pretend as if pack-objects didn't write files identical to ones that already exist, but rather that we respect what pack-objects wrote as the source of truth. That cuts two ways: - If pack-objects produced an identical pack to one that already exists with a bitmap, but did not produce a bitmap, we remove the bitmap that already exists. (This behavior is codified in t7700.14). - If pack-objects produced an identical pack to one that already exists, we trust the just-written version of the coresponding .idx, .promisor, and other files over the ones that already exist. This ensures that we use the most up-to-date versions of this files, which is safe even in the face of format changes in, say, the .idx file (which would not be reflected in the .idx file's name). Helped-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-16pull: check for local submodule modifications with the right rangeLibravatar Philippe Blain1-1/+1
Ever since 'git pull' learned '--recurse-submodules' in a6d7eb2c7a (pull: optionally rebase submodules (remote submodule changes only), 2017-06-23), we check if there are local submodule modifications by checking the revision range 'curr_head --not rebase_fork_point'. The goal of this check is to abort the pull if there are submodule modifications in the local commits being rebased, since this scenario is not supported. However, the actual range of commits being rebased is not 'rebase_fork_point..curr_head', as the logic in 'get_rebase_newbase_and_upstream' reveals, it is 'upstream..curr_head'. If the 'git merge-base --fork-point' invocation in 'get_rebase_fork_point' fails to find a fork point between the current branch and the remote-tracking branch we are pulling from, 'rebase_fork_point' is null and since 4d36f88be7 (submodule: do not pass null OID to setup_revisions, 2018-05-24), 'submodule_touches_in_range' checks 'curr_head' and all its ancestors for submodule modifications. Since it is highly likely that there are submodule modifications in this range (which is in effect the whole history of the current branch), this prevents 'git pull --rebase --recurse-submodules' from succeeding if no fork point exists between the current branch and the remote-tracking branch being pulled. This can happen, for example, when the current branch was forked from a commit which was never recorded in the reflog of the remote-tracking branch we are pulling, as the last two paragraphs of the "Discussion on fork-point mode" section in git-merge-base(1) explain. Fix this bug by passing 'upstream' instead of 'rebase_fork_point' as the 'excl_oid' argument to 'submodule_touches_in_range'. Reported-by: Brice Goglin <bgoglin@free.fr> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-16pull --rebase: compute rebase arguments in separate functionLibravatar Philippe Blain1-11/+35
The function 'run_rebase' is responsible for constructing the command line to be passed to 'git rebase'. This includes both forwarding pass-through options given to 'git pull' as well computing the <newbase> and <upstream> arguments to 'git rebase'. A following commit will need to access the <upstream> argument in 'cmd_pull' to fix a bug with 'git pull --rebase --recurse-submodules'. In order to do so, refactor the code so that the <newbase> and <upstream> commits are computed in a new, separate function, 'get_rebase_newbase_and_upstream'. Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-16builtin/repack.c: keep track of what pack-objects wroteLibravatar Taylor Blau1-1/+30
In the subsequent commit, it will become useful to keep track of which metadata files were written by pack-objects. We already do this to an extent with the 'exts' array, which only is used in the context of existing packs. Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-16repack: make "exts" array available outside cmd_repack()Libravatar Jeff King1-9/+10
We'll use it in a helper function soon. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-16use size_t to store pack .idx byte offsetsLibravatar Jeff King1-3/+3
We sometimes store the offset into a pack .idx file as an "unsigned long", but the mmap'd size of a pack .idx file can exceed 4GB. This is sufficient on LP64 systems like Linux, but will be too small on LLP64 systems like Windows, where "unsigned long" is still only 32 bits. Let's use size_t, which is a better type for an offset into a memory buffer. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-16compute pack .idx byte offsets using size_tLibravatar Jeff King1-1/+1
A pack and its matching .idx file are limited to 2^32 objects, because the pack format contains a 32-bit field to store the number of objects. Hence we use uint32_t in the code. But the byte count of even a .idx file can be much larger than that, because it stores at least a hash and an offset for each object. So using SHA-1, a v2 .idx file will cross the 4GB boundary at 153,391,650 objects. This confuses load_idx(), which computes the minimum size like this: unsigned long min_size = 8 + 4*256 + nr*(hashsz + 4 + 4) + hashsz + hashsz; Even though min_size will be big enough on most 64-bit platforms, the actual arithmetic is done as a uint32_t, resulting in a truncation. We actually exceed that min_size, but then we do: unsigned long max_size = min_size; if (nr) max_size += (nr - 1)*8; to account for the variable-sized table. That computation doesn't overflow quite so low, but with the truncation for min_size, we end up with a max_size that is much smaller than our actual size. So we complain that the idx is invalid, and can't find any of its objects. We can fix this case by casting "nr" to a size_t, which will do the multiplication in 64-bits (assuming you're on a 64-bit platform; this will never work on a 32-bit system since we couldn't map the whole .idx anyway). Likewise, we don't have to worry about further additions, because adding a smaller number to a size_t will convert the other side to a size_t. A few notes: - obviously we could just declare "nr" as a size_t in the first place (and likewise, packed_git.num_objects). But it's conceptually a uint32_t because of the on-disk format, and we correctly treat it that way in other contexts that don't need to compute byte offsets (e.g., iterating over the set of objects should and generally does use a uint32_t). Switching to size_t would make all of those other cases look wrong. - it could be argued that the proper type is off_t to represent the file offset. But in practice the .idx file must fit within memory, because we mmap the whole thing. And the rest of the code (including the idx_size variable we're comparing against) uses size_t. - we'll add the same cast to the max_size arithmetic line. Even though we're adding to a larger type, which will convert our result, the multiplication is still done as a 32-bit value and can itself overflow. I didn't check this with my test case, since it would need an even larger pack (~530M objects), but looking at compiler output shows that it works this way. The standard should agree, but I couldn't find anything explicit in 6.3.1.8 ("usual arithmetic conversions"). The case in load_idx() was the most immediate one that I was able to trigger. After fixing it, looking up actual objects (including the very last one in sha1 order) works in a test repo with 153,725,110 objects. That's because bsearch_hash() works with uint32_t entry indices, and the actual byte access: int cmp = hashcmp(table + mi * stride, sha1); is done with "stride" as a size_t, causing the uint32_t "mi" to be promoted to a size_t. This is the way most code will access the index data. However, I audited all of the other byte-wise accesses of packed_git.index_data, and many of the others are suspect (they are similar to the max_size one, where we are adding to a properly sized offset or directly to a pointer, but the multiplication in the sub-expression can overflow). I didn't trigger any of these in practice, but I believe they're potential problems, and certainly adding in the cast is not going to hurt anything here. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-11Merge branch 'rs/clear-commit-marks-in-repo'Libravatar Junio C Hamano2-2/+2
Code clean-up. * rs/clear-commit-marks-in-repo: bisect: clear flags in passed repository object: allow clear_commit_marks_all to handle any repo
2020-11-11shortlog: use strset from strmap.hLibravatar Elijah Newren1-57/+4
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-11Use new HASHMAP_INIT macro to simplify hashmap initializationLibravatar Elijah Newren1-5/+4
Now that hashamp has lazy initialization and a HASHMAP_INIT macro, hashmaps allocated on the stack can be initialized without a call to hashmap_init() and in some cases makes the code a bit shorter. Convert some callsites over to take advantage of this. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-11receive-pack: use default version 0 for proc-receiveLibravatar Jiang Xin1-1/+6
In the verison negotiation phase between "receive-pack" and "proc-receive", "proc-receive" can send an empty flush-pkt to end the negotiation and use default version 0. Capabilities (such as "push-options") are not supported in version 0. Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-11receive-pack: gently write messages to proc-receiveLibravatar Jiang Xin1-24/+63
Johannes found a flaky hang in `t5411/test-0013-bad-protocol.sh` in the osx-clang job of the CI/PR builds, and ran into an issue when using the `--stress` option with the following error messages: fatal: unable to write flush packet: Broken pipe send-pack: unexpected disconnect while reading sideband packet fatal: the remote end hung up unexpectedly In this test case, the "proc-receive" hook sends an error message and dies earlier. While "receive-pack" on the other side of the pipe should forward the error message of the "proc-receive" hook to the client side, but it fails to do so. This is because "receive-pack" uses `packet_write_fmt()` and `packet_flush()` to write pkt-line message to "proc-receive" hook, and these functions die immediately when pipe is broken. Using "gently" forms for these functions will get more predicable output. Add more "--die-*" options to test helper to test different stages of the protocol between "receive-pack" and "proc-receive" hook. Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-10rev-parse: handle --end-of-optionsLibravatar Jeff King1-23/+33
We taught rev-list a new way to separate options from revisions in 19e8789b23 (revision: allow --end-of-options to end option parsing, 2019-08-06), but rev-parse uses its own parser. It should know about --end-of-options not only for consistency, but because it may be presented with similarly ambiguous cases. E.g., if a caller does: git rev-parse "$rev" -- "$path" to parse an untrusted input, then it will get confused if $rev contains an option-like string like "--local-env-vars". Or even "--not-real", which we'd keep as an option to pass along to rev-list. Or even more importantly: git rev-parse --verify "$rev" can be confused by options, even though its purpose is safely parsing untrusted input. On the plus side, it will always fail the --verify part, as it will not have parsed a revision, so the caller will generally "fail closed" rather than continue to use the untrusted string. But it will still trigger whatever option was in "$rev"; this should be mostly harmless, since rev-parse options are all read-only, but I didn't carefully audit all paths. This patch lets callers write: git rev-parse --end-of-options "$rev" -- "$path" and: git rev-parse --verify --end-of-options "$rev" which will both treat "$rev" always as a revision parameter. The latter is a bit clunky. It would be nicer if we had defined "--verify" to require that its next argument be the revision. But we have not historically done so, and: git rev-parse --verify -q "$rev" does currently work. I added a test here to confirm that we didn't break that. A few implementation notes: - We don't document --end-of-options explicitly in commands, but rather in gitcli(7). So I didn't give it its own section in git-rev-parse(1). But I did call it out specifically in the --verify section, and include it in the examples, which should show best practices. - We don't have to re-indent the main option-parsing block, because we can combine our "did we see end of options" check with "does it start with a dash". The exception is the pre-setup options, which need their own block. - We do however have to pull the "--" parsing out of the "does it start with dash" block, because we want to parse it even if we've seen --end-of-options. - We'll leave "--end-of-options" in the output. This is probably not technically necessary, as a careful caller will do: git rev-parse --end-of-options $revs -- $paths and anything in $revs will be resolved to an object id. However, it does help a slightly less careful caller like: git rev-parse --end-of-options $revs_or_paths where a path "--foo" will remain in the output as long as it also exists on disk. In that case, it's helpful to retain --end-of-options to get passed along to rev-list, s it would otherwise see just "--foo". Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-10rev-parse: put all options under the "-" checkLibravatar Jeff King1-24/+23
The option-parsing loop of rev-parse checks whether the first character of an arg is "-". If so, then it enters a series of conditionals checking for individual options. But some options are inexplicably outside of that outer conditional. This doesn't produce the wrong behavior; the conditional is actually redundant with the individual option checks, and it's really only its fallback "continue" that we care about. But we should at least be consistent. One obvious alternative is that we could get rid of the conditional entirely. But we'll be using the extra block it provides in the next patch. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-10rev-parse: don't accept options after dashdashLibravatar Jeff King1-5/+6
Because of the order in which we check options in rev-parse, there are a few options we accept even after a "--". This is wrong, because the whole point of "--" is to say "everything after here is a path". Let's move the "did we see a dashdash" check (it's called "as_is" in the code) to the top of the parsing loop. Note there is one subtlety here. The options are ordered so that some are checked before we even see if we're in a repository (they continue the loop, and if we get past a certain point, then we do the repository setup). By moving the as_is check higher, it's also in that "before setup" section, even though it might look at the repository via verify_filename(). However, this works out: we'd never set as_is until we parse "--", and we don't parse that until after doing the setup. An alternative here to avoid the subtlety is to put the as_is check at the top of the post-setup options. But then every pre-setup option would have to remember to check "if (!as_is && !strcmp(...))". So while this is a bit magical, it's harder for future code to get wrong. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-09format-patch: make output filename configurableLibravatar Junio C Hamano1-6/+14
For the past 15 years, we've used the hardcoded 64 as the length limit of the filename of the output from the "git format-patch" command. Since the value is shorter than the 80-column terminal, it could grow without line wrapping a bit. At the same time, since the value is longer than half of the 80-column terminal, we could fit two or more of them in "ls" output on such a terminal if we allowed to lower it. Introduce a new command line option --filename-max-length=<n> and a new configuration variable format.filenameMaxLength to override the hardcoded default. While we are at it, remove a check that the name of output directory does not exceed PATH_MAX---this check is pointless in that by the time control reaches the function, the caller would already have done an equivalent of "mkdir -p", so if the system does not like an overly long directory name, the control wouldn't have reached here, and otherwise, we know that the system allowed the output directory to exist. In the worst case, we will get an error when we try to open the output file and handle the error correctly anyway. Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-09Merge branch 'jk/committer-date-is-author-date-fix-simplify'Libravatar Junio C Hamano1-16/+3
Code simplification. * jk/committer-date-is-author-date-fix-simplify: am, sequencer: stop parsing our own committer ident
2020-11-09Merge branch 'ab/git-remote-exit-code'Libravatar Junio C Hamano1-14/+28
Exit codes from "git remote add" etc. were not usable by scripted callers. * ab/git-remote-exit-code: remote: add meaningful exit code on missing/existing
2020-11-09Merge branch 'jk/checkout-index-errors'Libravatar Junio C Hamano1-2/+14
"git checkout-index" did not consistently signal an error with its exit status. * jk/checkout-index-errors: checkout-index: propagate errors to exit code checkout-index: drop error message from empty --stage=all
2020-11-09Merge branch 'mr/bisect-in-c-3'Libravatar Junio C Hamano1-76/+111
Rewriting "git bisect" in C continues. * mr/bisect-in-c-3: bisect--helper: retire `--bisect-autostart` subcommand bisect--helper: retire `--write-terms` subcommand bisect--helper: retire `--check-expected-revs` subcommand bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C bisect--helper: retire `--next-all` subcommand bisect--helper: retire `--bisect-clean-state` subcommand bisect--helper: finish porting `bisect_start()` to C
2020-11-04rebase -i: simplify get_revision_ranges()Libravatar Phillip Wood1-6/+4
Now that all the external users of head_hash have been converted to use a opts->orig_head instead we can stop returning head_hash from get_revision_ranges(). Because we want to pass the full object names back to the caller in `revisions` the find_unique_abbrev_r() call that was used to initialize `head_hash` is replaced with oid_to_hex(). Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-04rebase -i: use struct object_id when writing stateLibravatar Phillip Wood1-2/+3
Rather than passing a string around pass the struct object_id that the string was created from call oid_hex() when we write the file. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-04rebase -i: use struct object_id rather than looking up commitLibravatar Phillip Wood1-2/+3
We already have a struct object_id containing the oid that we want to set ORIG_HEAD to so use that rather than converting it to a string and then calling get_oid() on that string. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>