This helps format-patch gain completion for a couple new options,
notably --range-diff.
Since send-email completion relies on $__git_format_patch_options
which is now reduced, we need to do something not to regress
send-email completion.
The workaround here is implement --git-completion-helper in
send-email.perl just as a bridge to "format-patch --git-completion-helper".
This is enough to use __gitcomp_builtin on send-email (to take
advantage of caching).
In the end, send-email.perl can probably reuse the same info it passes
to GetOptions() to generate full --git-completion-helper output so
that we don't need to keep track of its options in git-completion.bash
anymore. But that's something for another boring day.
Helped-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
With refresh_index() learning to utilize preload_index() to speed up its
operation there is no longer any benefit to having the caller preload the
index first. Remove those unneeded calls by calling read_index() instead of
the preload variant.
There is no measurable performance impact of this patch - the 2nd call to
preload_index() bails out quickly but there is no reason to call it twice.
Signed-off-by: Ben Peart <benpeart@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This function was used only for parsing the hunk headers generated by
xdiff. Now that we can use hunk callbacks to get that information
directly, it has outlived its usefulness.
Note to anyone who wants to resurrect it: the "len" parameter was
totally unused, meaning that the function could read past the end of the
"line" array. In practice this never happened, because we only used it
to parse xdiff's generated header lines. But it would be dangerous to
use it for other cases without fixing this defect.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we count the lines in a diff, we don't actually care about the
contents of each line. By using a hunk callback, we tell xdiff that it
does not need to even bother generating a hunk header line, saving a
small amount of work.
Arguably we could even ignore the hunk headers completely, since we're
just computing a cost function between patches. But doing it this way
maintains the exact same behavior before and after.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "diff --check" code needs to know the line number on which each hunk
starts in order to generate its output. We get that now by parsing the
hunk header line generated by xdiff, but it's much simpler to just pass
it directly using a hunk callback.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A combined diff has to line up the hunks for all of the individual
pairwise diffs, and thus needs to know their line numbers and sizes. We
get that now by parsing the hunk header line that xdiff generates.
However, now that xdiff supports a hunk callback, we can just use the
values directly.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Our word-diff does not look at the -/+ lines generated by xdiff at all
(because they are not real lines to show the user, but just the
tokenized words split into lines). Instead we use the line numbers from
the hunk headers to index our own data structure.
As a result, our xdi_diff_outf() callback throws away all lines except
hunk headers. We can instead use a hunk callback, which has two
benefits:
1. We don't have to re-parse the generated hunk header line, but can
use the passed parameters directly.
2. By setting our line callback to NULL, we can tell xdiff-interface
that it does not even need to bother generating the other lines,
saving a small amount of work.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We do not include hunk header lines when computing patch-ids, since
the line numbers would create false negatives. Rather than detect and
skip them in our line callback, we can simply tell xdiff to avoid
generating them.
This is similar to the previous commit, but split out because it
actually requires modifying the matching line callback.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some callers of xdi_diff_outf() do not look at the generated hunk header
lines at all. By plugging in a no-op hunk callback, this tells xdiff not
to even bother formatting them.
This patch introduces a stock no-op callback and uses it with a few
callers whose line callbacks explicitly ignore hunk headers (because
they look only for +/- lines).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The value of timeout starts as an int value, and for this reason it
cannot overflow unsigned long long aka ULONGLONG. The unsigned version
of this initial value is available in orig_timeout. The difference
(orig_timeout - elapsed) cannot wrap around because it is protected by
a conditional (as can be seen in the patch text). Hence, the ULONGLONG
difference can only have values that are smaller than the initial
timeout value and truncation to int cannot overflow.
Signed-off-by: Steve Hoelzer <shoelzer@gmail.com>
[j6t: improved both implementation and log message]
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
tree_entry_interesting() is used for matching pathspec on a tree. The
interesting thing about this function is that, because the tree
entries are known to be sorted, this function can return more than
just "yes, matched" and "no, not matched". It can also say "yes, this
entry is matched and so is the remaining entries in the tree".
This is where I made a mistake when matching exclude pathspec. For
exclude pathspec, we do matching twice, one with positive patterns and
one with negative ones, then a rule table is applied to determine the
final "include or exclude" result. Note that "matched" does not
necessarily mean include. For negative patterns, "matched" means
exclude.
This particular rule is too eager to include everything. Rule 8 says
that "if all entries are positively matched" and the current entry is
not negatively matched (i.e. not excluded), then all entries are
positively matched and therefore included. But this is not true. If
the _current_ entry is not negatively matched, it does not mean the
next one will not be and we cannot conclude right away that all
remaining entries are positively matched and can be included.
Rules 8 and 18 are now updated to be less eager. We conclude that the
current entry is positively matched and included. But we say nothing
about remaining entries. tree_entry_interesting() will be called again
for those entries where we will determine entries individually.
Reported-by: Christophe Bliard <christophe.bliard@trux.info>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Noticed-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
On platforms with recent cURL library, http.sslBackend configuration
variable can be used to choose a different SSL backend at runtime.
The Windows port uses this mechanism to switch between OpenSSL and
Secure Channel while talking over the HTTPS protocol.
* js/mingw-http-ssl:
http: when using Secure Channel, ignore sslCAInfo by default
http: add support for disabling SSL revocation checks in cURL
http: add support for selecting SSL backends at runtime
New "--pretty=format:" placeholders %GF and %GP that show the GPG
key fingerprints have been invented.
* mg/gpg-fingerprint:
gpg-interface.c: obtain primary key fingerprint as well
gpg-interface.c: support getting key fingerprint via %GF format
gpg-interface.c: use flags to determine key/signer info presence
Detect and reject a signature block that has more than one GPG
signature.
* mg/gpg-parse-tighten:
gpg-interface.c: detect and reject multiple signatures on commits
Further clean-up of merge-recursive machinery.
* en/merge-cleanup-more:
merge-recursive: avoid showing conflicts with merge branch before HEAD
merge-recursive: improve auto-merging messages with path collisions
During an "add", a call is made to run_diff_files() which calls
check_removed() for each index-entry. The preload_index() code
distributes some of the costs across multiple threads.
Because the files checked are restricted to pathspec, adding
individual files makes no measurable impact but on a Windows repo
with ~200K files, 'git add .' drops from 6.3 seconds to 3.3 seconds
for a 47% savings.
Signed-off-by: Ben Peart <benpeart@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The add_missing_tags() method currently has quadratic behavior.
This is due to a linear number (based on number of tags T) of
calls to in_merge_bases_many, which has linear performance (based
on number of commits C in the repository).
Replace this O(T * C) algorithm with an O(T + C) algorithm by
using get_reachable_subset(). We ignore the return list and focus
instead on the reachable_flag assigned to the commits we care
about, because we need to interact with the tag ref and not just
the commit object.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The get_reachable_subset() method returns the list of commits in
the 'to' array that are reachable from at least one commit in the
'from' array. Add tests that check this method works in a few
cases:
1. All commits in the 'to' list are reachable. This exercises the
early-termination condition.
2. Some commits in the 'to' list are reachable. This exercises the
loop-termination condition.
3. No commits in the 'to' list are reachable. This exercises the
NULL return condition.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The existing reachability algorithms in commit-reach.c focus on
finding merge-bases or determining if all commits in a set X can
reach at least one commit in a set Y. However, for two commits sets
X and Y, we may also care about which commits in Y are reachable
from at least one commit in X.
Implement get_reachable_subset() which answers this question. Given
two arrays of commits, 'from' and 'to', return a commit_list with
every commit from the 'to' array that is reachable from at least
one commit in the 'from' array.
The algorithm is a simple walk starting at the 'from' commits, using
the PARENT2 flag to indicate "this commit has already been added to
the walk queue". By marking the 'to' commits with the PARENT1 flag,
we can determine when we see a commit from the 'to' array. We remove
the PARENT1 flag as we add that commit to the result list to avoid
duplicates.
The order of the resulting list is a reverse of the order that the
commits are discovered in the walk.
There are a couple shortcuts to avoid walking more than we need:
1. We determine the minimum generation number of commits in the
'to' array. We do not walk commits with generation number
below this minimum.
2. We count how many distinct commits are in the 'to' array, and
decrement this count when we discover a 'to' commit during the
walk. If this number reaches zero, then we can terminate the
walk.
Tests will be added using the 'test-tool reach' helper in a
subsequent commit.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fix a small bug introduced by "7a36987ff (send-email: add an auto option
for transfer encoding, 2018-07-14)".
I saw the following message when setting --transfer-encoding for a file
with the same encoding:
$ git send-email --transfer-encoding=8bit example.patch
Use of uninitialized value $xfer_encoding in concatenation (.) or string
at /usr/lib/git-core/git-send-email line 1744.
The new tests are by brian m. carlson.
Signed-off-by: Aaron Lindsay <aaron@aclindsay.com>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The pathspec code always takes names to be matched as a
name/namelen pair, but match_attrs() never looks at namelen,
and just treats "name" like a NUL-terminated string, passing
it to git_check_attr().
This usually works anyway. Every caller passes a
NUL-terminated string, and in all but one the passed-in
length is the same as the length of the string (the
exception is dir_path_match(), which may pass a smaller
length to drop a trailing slash). So we won't currently ever
read random memory, and the one case I found actually
happens to work correctly because the attr code can handle
the trailing slash itself.
But it's still worth addressing, as the function interface
implies that the name does not have to be NUL-terminated,
making this an accident waiting to happen.
Since teaching git_check_attr() to take a ptr/len pair would
be a big refactor, we'll just allocate a new string. We can
do this only when necessary, which avoids paying the cost
for most callers.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The approxidate parser has a table of special keywords like
"yesterday", "noon", "pm", etc. Some of these, like "pm", do
the right thing if we've recently seen a number: "3pm" is
what you'd think.
However, most of them do not look at or modify the
pending-number flag at all, which means a number may "jump"
across a significant keyword and be used unexpectedly. For
example, when parsing:
January 5th noon pm
we'd connect the "5" to "pm", and ignore it as a
day-of-month. This is obviously a bit silly, as "noon"
already implies "pm". And other mis-parsed things are
generally as silly ("January 5th noon, years ago" would
connect the 5 to "years", but probably nobody would type
that).
However, the fix is simple: when we see a keyword like
"noon", we should flush the pending number (as we would if
we hit another number, or the end of the string). In a few
of the specials that actually modify the day, we can simply
throw away the number (saying "Jan 5 yesterday" should not
respect the number at all).
Note that we have to either move or forward-declare the
static pending_number() to make it accessible to these
functions; this patch moves it.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a traversal sees the --indexed-objects option, it adds
all blobs and valid cache-trees from the index to the
traversal using add_index_objects_to_pending(). But that
function totally ignores its flags parameter!
That means that doing:
git rev-list --objects --indexed-objects
and
git rev-list --objects --not --indexed-objects
produce the same output, because we ignore the UNINTERESTING
flag when walking the index in the second example.
Nobody noticed because this feature was added as a way for
tools like repack to increase their coverage of reachable
objects, meaning it would only be used like the first
example above.
But since it's user facing (and because the documentation
describes it "as if the objects are listed on the command
line"), we should make sure the negative case behaves
sensibly.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The previous commit taught xdiff to optionally provide the hunk header
data to a specialized callback. But most users of xdiff actually use our
more convenient xdi_diff_outf() helper, which ensures that our callbacks
are always fed whole lines.
Let's plumb the special hunk-callback through this interface, too. It
will follow the same rule as xdiff when the hunk callback is NULL (i.e.,
continue to pass a stringified hunk header to the line callback). Since
we add NULL to each caller, there should be no behavior change yet.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The xdiff library always emits hunk header lines to our callbacks as
formatted strings like "@@ -a,b +c,d @@\n". This is convenient if we're
going to output a diff, but less so if we actually need to compute using
those numbers, which requires re-parsing the line.
In preparation for moving away from this, let's teach xdiff a new
callback function which gets the broken-out hunk information. To help
callers that don't want to use this new callback, if it's NULL we'll
continue to format the hunk header into a string.
Note that this function renames the "outf" callback to "out_line", as
well. This isn't strictly necessary, but helps in two ways:
1. Now that there are two callbacks, it's nice to use more descriptive
names.
2. Many callers did not zero the emit_callback_data struct, and needed
to be modified to set ecb.out_hunk to NULL. By changing the name of
the existing struct member, that guarantees that any new callers
from in-flight topics will break the build and be examined
manually.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ever since we started using Travis CI, we specified the list of
packages to install in '.travis.yml' via the APT addon. While running
our builds on Travis CI's container-based infrastructure we didn't
have another choice, because that environment didn't support 'sudo',
and thus we didn't have permission to install packages ourselves. With
the switch to the VM-based infrastructure in the previous patch we do
get a working 'sudo', so we can install packages by running 'sudo
apt-get -y install ...' as well.
Let's make use of this and install necessary packages in
'ci/install-dependencies.sh', so all the dependencies (i.e. both
packages and "non-packages" (P4 and Git-LFS)) are handled in the same
file. Install gcc-8 only in the 'linux-gcc' build job; so far it has
been unnecessarily installed in the 'linux-clang' build job as well.
Print the versions of P4 and Git-LFS conditionally, i.e. only when
they have been installed; with this change even the static analysis
and documentation build jobs start using 'ci/install-dependencies.sh'
to install packages, and neither of these two build jobs depend on and
thus install those.
This change will presumably be beneficial for the upcoming Azure
Pipelines integration [1]: preliminary versions of that patch series
run a couple of 'apt-get' commands to install the necessary packages
before running 'ci/install-dependencies.sh', but with this patch it
will be sufficient to run only 'ci/install-dependencies.sh'.
[1] https://public-inbox.org/git/1a22efe849d6da79f2c639c62a1483361a130238.1539598316.git.gitgitgadget@gmail.com/
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `--preserve-merges` mode of the `rebase` command is slated to be
deprecated soon, as the more powerful `--rebase-merges` mode is
available now, and the latter was designed with the express intent to
address the shortcomings of `--preserve-merges`' design (e.g. the
inability to reorder commits in an interactive rebase).
As such, we will eventually even remove the `--preserve-merges` support,
and along with it, its tests.
In preparation for this, and also to allow the Windows phase of our
automated tests to save some well-needed time when running the test
suite, this commit introduces a new prerequisite REBASE_P, which can be
forced to being unmet by setting the environment variable
`GIT_TEST_SKIP_REBASE_P` to any non-empty string.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
It is in general a good idea for regression test cases to be as
independent of each other as possible (with the one exception of an
initial `setup` test case, which is only a test case in Git's test suite
because it does not have a notion of a fixture or setup).
This patch addresses one particular instance of this principle being
violated: a few test cases in t3418-rebase-continue.sh depend on a side
effect of a test case that verifies a specific `rebase -p` behavior. The
later test cases should, however, still succeed even if the `rebase -p`
test case is skipped.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Originally, the `--preserve-merges` option of the `git rebase` command
piggy-backed on top of the `--interactive` feature. For that reason, the
early test cases were added to the very same test script that contains
the `git rebase -i` tests: `t3404-rebase-interactive.sh`.
However, since c42abfe785 (rebase: introduce a dedicated backend for
--preserve-merges, 2018-05-28), the `--preserve-merges` feature got its
own backend, in preparation for converting the rest of the
`--interactive` code to built-in code, written in C rather than shell.
The reason why the `--preserve-merges` feature was not converted at the
same time is that we have something much better now: `--rebase-merges`.
That option intends to supersede `--preserve-merges`, and we will
probably deprecate the latter soon.
Once `--preserve-merges` has been deprecated for a good amount of time,
it will be time to remove it, and along with it, its tests.
In preparation for that, let's make the rest of the test cases in
`t3404-rebase-interactive.sh` independent of the test cases dedicated to
`--preserve-merges`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git rebase -i" learned to take 'b' as the short form of 'break'
option in the todo list.
* js/rebase-i-shortopt:
rebase -i: recognize short commands without arguments
"git rebase -i" learned a new insn, 'break', that the user can
insert in the to-do list. Upon hitting it, the command returns
control back to the user.
* js/rebase-i-break:
rebase -i: introduce the 'break' command
rebase -i: clarify what happens on a failed `exec`
"git rebase" that has recently been rewritten in C had a few issues
in its "--autstash" feature, which have been corrected.
* js/rebase-autostash-fix:
rebase --autostash: fix issue with dirty submodules
rebase --autostash: demonstrate a problem with dirty submodules
rebase (autostash): use an explicit OID to apply the stash
rebase (autostash): store the full OID in <state-dir>/autostash
rebase (autostash): avoid duplicate call to state_dir_path()
"rebase" that has been rewritten learns the new calling convention
used by "rebase -i" that was rewritten in C, tying the loose end
between two GSoC topics that stomped on each other's toes.
* js/rebase-in-c-5.5-work-with-rebase-i-in-c:
builtin rebase: prepare for builtin rebase -i
Rewrite "git rebase" in C.
* pk/rebase-in-c-5-test:
builtin rebase: error out on incompatible option/mode combinations
builtin rebase: use no-op editor when interactive is "implied"
builtin rebase: show progress when connected to a terminal
builtin rebase: fast-forward to onto if it is a proper descendant
builtin rebase: optionally pass custom reflogs to reset_head()
builtin rebase: optionally auto-detect the upstream
Rewrite "git rebase" in C.
* pk/rebase-in-c-4-opts:
builtin rebase: support --root
builtin rebase: add support for custom merge strategies
builtin rebase: support `fork-point` option
merge-base --fork-point: extract libified function
builtin rebase: support --rebase-merges[=[no-]rebase-cousins]
builtin rebase: support `--allow-empty-message` option
builtin rebase: support `--exec`
builtin rebase: support `--autostash` option
builtin rebase: support `-C` and `--whitespace=<type>`
builtin rebase: support `--gpg-sign` option
builtin rebase: support `--autosquash`
builtin rebase: support `keep-empty` option
builtin rebase: support `ignore-date` option
builtin rebase: support `ignore-whitespace` option
builtin rebase: support --committer-date-is-author-date
builtin rebase: support --rerere-autoupdate
builtin rebase: support --signoff
builtin rebase: allow selecting the rebase "backend"
Rewrite "git rebase" in C.
* pk/rebase-in-c-3-acts:
builtin rebase: stop if `git am` is in progress
builtin rebase: actions require a rebase in progress
builtin rebase: support --edit-todo and --show-current-patch
builtin rebase: support --quit
builtin rebase: support --abort
builtin rebase: support --skip
builtin rebase: support --continue
Rewrite "git rebase" in C.
* pk/rebase-in-c-2-basic:
builtin rebase: support `git rebase <upstream> <switch-to>`
builtin rebase: only store fully-qualified refs in `options.head_name`
builtin rebase: start a new rebase only if none is in progress
builtin rebase: support --force-rebase
builtin rebase: try to fast forward when possible
builtin rebase: require a clean worktree
builtin rebase: support the `verbose` and `diffstat` options
builtin rebase: support --quiet
builtin rebase: handle the pre-rebase hook and --no-verify
builtin rebase: support `git rebase --onto A...B`
builtin rebase: support --onto
Rewrite of the remaining "rebase -i" machinery in C.
* ag/rebase-i-in-c:
rebase -i: move rebase--helper modes to rebase--interactive
rebase -i: remove git-rebase--interactive.sh
rebase--interactive2: rewrite the submodes of interactive rebase in C
rebase -i: implement the main part of interactive rebase as a builtin
rebase -i: rewrite init_basic_state() in C
rebase -i: rewrite write_basic_state() in C
rebase -i: rewrite the rest of init_revisions_and_shortrevisions() in C
rebase -i: implement the logic to initialize $revisions in C
rebase -i: remove unused modes and functions
rebase -i: rewrite complete_action() in C
t3404: todo list with commented-out commands only aborts
sequencer: change the way skip_unnecessary_picks() returns its result
sequencer: refactor append_todo_help() to write its message to a buffer
rebase -i: rewrite checkout_onto() in C
rebase -i: rewrite setup_reflog_action() in C
sequencer: add a new function to silence a command, except if it fails
rebase -i: rewrite the edit-todo functionality in C
editor: add a function to launch the sequence editor
rebase -i: rewrite append_todo_help() in C
sequencer: make three functions and an enum from sequencer.c public
Rewrite of the "rebase" machinery in C.
* pk/rebase-in-c:
builtin/rebase: support running "git rebase <upstream>"
rebase: refactor common shell functions into their own file
rebase: start implementing it as a builtin
In find_non_local_tags() helper function (used to implement the
"follow tags"), we use string_list_has_string() on two string lists
as a way to see if a refname has already been processed, etc.
All this code predates more modern in-core lookup API like hashmap;
replace them with two hashmaps and one string list---the hashmaps
are used for look-ups and the string list is to keep the order of
items in the returned result stable (which is the only single thing
hashmap does worse than lookups on string-list).
Similarly, get_ref_map() uses another string-list as a look-up table
for existing refs. Replace it with a hashmap.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commit [1] added the --exclude option to revision.c. The --all,
--branches, --tags, --remotes, and --glob options clear the exclude
list. Shortly therafter, commit [2] added the same to 'git rev-parse',
but without clearing the exclude list for the --all option.
[1] e7b432c52 ("revision: introduce --exclude=<glob> to tame wildcards", 2013-08-30)
[2] 9dc01bf06 ("rev-parse: introduce --exclude=<glob> to tame wildcards", 2013-11-01)
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Each section in a protocol v2 response is followed by either a DELIM
packet (indicating more sections to follow) or a FLUSH packet
(indicating none to follow). But when parsing the "acknowledgments"
section, do_fetch_pack_v2() is liberal in accepting both, but determines
whether to continue reading or not based solely on the contents of the
"acknowledgments" section, not on whether DELIM or FLUSH was read.
There is no issue with a protocol-compliant server, but can result in
confusing error messages when communicating with a server that
serves unexpected additional sections. Consider a server that sends
"new-section" after "acknowledgments":
- client writes request
- client reads the "acknowledgments" section which contains no "ready",
then DELIM
- since there was no "ready", client needs to continue negotiation, and
writes request
- client reads "new-section", and reports to the end user "expected
'acknowledgments', received 'new-section'"
For the person debugging the involved Git implementation(s), the error
message is confusing in that "new-section" was not received in response
to the latest request, but to the first one.
One solution is to always continue reading after DELIM, but in this
case, we can do better. We know from the protocol that "ready" means at
least the packfile section is coming (hence, DELIM) and that no "ready"
means that no sections are to follow (hence, FLUSH). So teach
process_acks() to enforce this.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Duy Nguyen
Ben Peart
Jeff King
Steve Hoelzer
Junio C Hamano
Derrick Stolee
Aaron Lindsay
SZEDER Gábor
Johannes Schindelin
Andreas Gruenbacher
Jonathan Tan