Certain characters are not admissible in file names on Windows, even if
Cygwin/MSYS2 (and therefore, Git for Windows' Bash) pretend that they
are, e.g. `:`, `<`, `>`, etc
Let's disallow those characters explicitly in Windows builds of Git.
Note: just like trailing spaces or periods, it _is_ possible on Windows
to create commits adding files with such illegal characters, as long as
the operation leaves the worktree untouched. To allow for that, we
continue to guard `is_valid_win32_path()` behind the config setting
`core.protectNTFS`, so that users _can_ continue to do that, as long as
they turn the protections off via that config setting.
Among other problems, this prevents Git from trying to write to an "NTFS
Alternate Data Stream" (which refers to metadata stored alongside a
file, under a special name: "<filename>:<stream-name>"). This fix
therefore also prevents an attack vector that was exploited in
demonstrations of a number of recently-fixed security bugs.
Further reading on illegal characters in Win32 filenames:
https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When the, say, 93rd trial run fails, it is a good idea to have a way to
skip the first 92 trials and dig directly into the 93rd in a debugger.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
On Windows, file names cannot contain asterisks nor newline characters.
In an upcoming commit, we will make this limitation explicit,
disallowing even the creation of commits that introduce such file names.
However, in the test scripts touched by this patch, we _know_ that those
paths won't be checked out, so we _want_ to allow such file names.
Happily, the stringent path validation will be guarded via the
`core.protectNTFS` flag, so all we need to do is to force that flag off
temporarily.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
When the stress test reported a problem with quoting certain arguments,
it is helpful to have a facility to play with those arguments in order
to find out whether variations of those arguments are affected, too.
Let's allow `test-run-command quote-stress-test -- <args>` to be used
for that purpose.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
On Windows, we have to do all the command-line argument quoting
ourselves. Worse: we have to have two versions of said quoting, one for
MSYS2 programs (which have their own dequoting rules) and the rest.
We care mostly about the rest, and to make sure that that works, let's
have a stress test that comes up with all kinds of awkward arguments,
verifying that a spawned sub-process receives those unharmed.
Signed-off-by: Garima Singh <garima.singh@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We need to be careful to follow proper quoting rules. For example, if an
argument contains spaces, we have to quote them. Double-quotes need to
be escaped. Backslashes need to be escaped, but only if they are
followed by a double-quote character.
We need to be _extra_ careful to consider the case where an argument
ends in a backslash _and_ needs to be quoted: in this case, we append a
double-quote character, i.e. the backslash now has to be escaped!
The current code, however, fails to recognize that, and therefore can
turn an argument that ends in a single backslash into a quoted argument
that now ends in an escaped double-quote character. This allows
subsequent command-line parameters to be split and part of them being
mistaken for command-line options, e.g. through a maliciously-crafted
submodule URL during a recursive clone.
Technically, we would not need to quote _all_ arguments which end in a
backslash _unless_ the argument needs to be quoted anyway. For example,
`test\` would not need to be quoted, while `test \` would need to be.
To keep the code simple, however, and therefore easier to reason about
and ensure its correctness, we now _always_ quote an argument that ends
in a backslash.
This addresses CVE-2019-1350.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Currently it is technically possible to let a submodule's git
directory point right into the git dir of a sibling submodule.
Example: the git directories of two submodules with the names `hippo`
and `hippo/hooks` would be `.git/modules/hippo/` and
`.git/modules/hippo/hooks/`, respectively, but the latter is already
intended to house the former's hooks.
In most cases, this is just confusing, but there is also a (quite
contrived) attack vector where Git can be fooled into mistaking remote
content for file contents it wrote itself during a recursive clone.
Let's plug this bug.
To do so, we introduce the new function `validate_submodule_git_dir()`
which simply verifies that no git dir exists for any leading directories
of the submodule name (if there are any).
Note: this patch specifically continues to allow sibling modules names
of the form `core/lib`, `core/doc`, etc, as long as `core` is not a
submodule name.
This fixes CVE-2019-1387.
Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
We just safe-guarded `.git` against NTFS Alternate Data Stream-related
attack vectors, and now it is time to do the same for `.gitmodules`.
Note: In the added regression test, we refrain from verifying all kinds
of variations between short names and NTFS Alternate Data Streams: as
the new code disallows _all_ Alternate Data Streams of `.gitmodules`, it
is enough to test one in order to know that all of them are guarded
against.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Probably inspired by HFS' resource streams, NTFS supports "Alternate
Data Streams": by appending `:<stream-name>` to the file name,
information in addition to the file contents can be written and read,
information that is copied together with the file (unless copied to a
non-NTFS location).
These Alternate Data Streams are typically used for things like marking
an executable as having just been downloaded from the internet (and
hence not necessarily being trustworthy).
In addition to a stream name, a stream type can be appended, like so:
`:<stream-name>:<stream-type>`. Unless specified, the default stream
type is `$DATA` for files and `$INDEX_ALLOCATION` for directories. In
other words, `.git::$INDEX_ALLOCATION` is a valid way to reference the
`.git` directory!
In our work in Git v2.2.1 to protect Git on NTFS drives under
`core.protectNTFS`, we focused exclusively on NTFS short names, unaware
of the fact that NTFS Alternate Data Streams offer a similar attack
vector.
Let's fix this.
Seeing as it is better to be safe than sorry, we simply disallow paths
referring to *any* NTFS Alternate Data Stream of `.git`, not just
`::$INDEX_ALLOCATION`. This also simplifies the implementation.
This closes CVE-2019-1352.
Further reading about NTFS Alternate Data Streams:
https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fscc/c54dec26-1551-4d3a-a0ea-4fa40f848eb3
Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In preparation to flipping the default on `core.protectNTFS`, let's have
some way to measure the speed impact of this config setting reliably
(and for comparison, the `core.protectHFS` config setting).
For now, this is a manual performance benchmark:
./t/helper/test-path-utils protect_ntfs_hfs [arguments...]
where the arguments are an optional number of file names to test with,
optionally followed by minimum and maximum length of the random file
names. The default values are one million, 3 and 20, respectively.
Just like `sqrti()` in `bisect.c`, we introduce a very simple function
to approximation the square root of a given value, in order to avoid
having to introduce the first user of `<math.h>` in Git's source code.
Note: this is _not_ implemented as a Unix shell script in t/perf/
because we really care about _very_ precise timings here, and Unix shell
scripts are simply unsuited for precise and consistent benchmarking.
Signed-off-by: Garima Singh <garima.singh@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Since we're now recommending lore.kernel.org, replace marc.info links
with lore.kernel.org.
Although MARC has been around for a long time, nothing lasts forever
(see Gmane). Since MARC uses opaque message identifiers, switching to
lore.kernel.org should be a strict improvement since, even if
lore.kernel.org goes down, the Message-ID will allow future readers to
look up the referenced messages on any other archive.
We leave behind one reference to MARC in the README.md since it's a
perfectly fine mail archive for personal reading, just not for linking
messages for the future.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The backslash character is not a valid part of a file name on Windows.
Hence it is dangerous to allow writing files that were unpacked from
tree objects, when the stored file name contains a backslash character:
it will be misinterpreted as directory separator.
This not only causes ambiguity when a tree contains a blob `a\b` and a
tree `a` that contains a blob `b`, but it also can be used as part of an
attack vector to side-step the careful protections against writing into
the `.git/` directory during a clone of a maliciously-crafted
repository.
Let's prevent that, addressing CVE-2019-1354.
Note: we guard against backslash characters in tree objects' file names
_only_ on Windows (because on other platforms, even on those where NTFS
volumes can be mounted, the backslash character is _not_ a directory
separator), and _only_ when `core.protectNTFS = true` (because users
might need to generate tree objects for other platforms, of course
without touching the worktree, e.g. using `git update-index
--cacheinfo`).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In addition to preventing `.git` from being tracked by Git, on Windows
we also have to prevent `git~1` from being tracked, as the default NTFS
short name (also known as the "8.3 filename") for the file name `.git`
is `git~1`, otherwise it would be possible for malicious repositories to
write directly into the `.git/` directory, e.g. a `post-checkout` hook
that would then be executed _during_ a recursive clone.
When we implemented appropriate protections in 2b4c6efc82 (read-cache:
optionally disallow NTFS .git variants, 2014-12-16), we had analyzed
carefully that the `.git` directory or file would be guaranteed to be
the first directory entry to be written. Otherwise it would be possible
e.g. for a file named `..git` to be assigned the short name `git~1` and
subsequently, the short name generated for `.git` would be `git~2`. Or
`git~3`. Or even `~9999999` (for a detailed explanation of the lengths
we have to go to protect `.gitmodules`, see the commit message of
e7cb0b4455 (is_ntfs_dotgit: match other .git files, 2018-05-11)).
However, by exploiting two issues (that will be addressed in a related
patch series close by), it is currently possible to clone a submodule
into a non-empty directory:
- On Windows, file names cannot end in a space or a period (for
historical reasons: the period separating the base name from the file
extension was not actually written to disk, and the base name/file
extension was space-padded to the full 8/3 characters, respectively).
Helpfully, when creating a directory under the name, say, `sub.`, that
trailing period is trimmed automatically and the actual name on disk
is `sub`.
This means that while Git thinks that the submodule names `sub` and
`sub.` are different, they both access `.git/modules/sub/`.
- While the backslash character is a valid file name character on Linux,
it is not so on Windows. As Git tries to be cross-platform, it
therefore allows backslash characters in the file names stored in tree
objects.
Which means that it is totally possible that a submodule `c` sits next
to a file `c\..git`, and on Windows, during recursive clone a file
called `..git` will be written into `c/`, of course _before_ the
submodule is cloned.
Note that the actual exploit is not quite as simple as having a
submodule `c` next to a file `c\..git`, as we have to make sure that the
directory `.git/modules/b` already exists when the submodule is checked
out, otherwise a different code path is taken in `module_clone()` that
does _not_ allow a non-empty submodule directory to exist already.
Even if we will address both issues nearby (the next commit will
disallow backslash characters in tree entries' file names on Windows,
and another patch will disallow creating directories/files with trailing
spaces or periods), it is a wise idea to defend in depth against this
sort of attack vector: when submodules are cloned recursively, we now
_require_ the directory to be empty, addressing CVE-2019-1349.
Note: the code path we patch is shared with the code path of `git
submodule update --init`, which must not expect, in general, that the
directory is empty. Hence we have to introduce the new option
`--force-init` and hand it all the way down from `git submodule` to the
actual `git submodule--helper` process that performs the initial clone.
Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
As with export-marks in the previous commit, import-marks can access the
filesystem. This is significantly less dangerous than export-marks
because it only involves reading from arbitrary paths, rather than
writing them. However, it could still be surprising and have security
implications (e.g., exfiltrating data from a service that accepts
fast-import streams).
Let's lump it (and its "if-exists" counterpart) in with export-marks,
and enable the in-stream version only if --allow-unsafe-features is set.
Signed-off-by: Jeff King <peff@peff.net>
The fast-import stream command "feature export-marks=<path>" lets the
stream write marks to an arbitrary path. This may be surprising if you
are running fast-import against an untrusted input (which otherwise
cannot do anything except update Git objects and refs).
Let's disallow the use of this feature by default, and provide a
command-line option to re-enable it (you can always just use the
command-line --export-marks as well, but the in-stream version provides
an easy way for exporters to control the process).
This is a backwards-incompatible change, since the default is flipping
to the new, safer behavior. However, since the main users of the
in-stream versions would be import/export-based remote helpers, and
since we trust remote helpers already (which are already running
arbitrary code), we'll pass the new option by default when reading a
remote helper's stream. This should minimize the impact.
Note that the implementation isn't totally simple, as we have to work
around the fact that fast-import doesn't parse its command-line options
until after it has read any "feature" lines from the stream. This is how
it lets command-line options override in-stream. But in our case, it's
important to parse the new --allow-unsafe-features first.
There are three options for resolving this:
1. Do a separate "early" pass over the options. This is easy for us to
do because there are no command-line options that allow the
"unstuck" form (so there's no chance of us mistaking an argument
for an option), though it does introduce a risk of incorrect
parsing later (e.g,. if we convert to parse-options).
2. Move the option parsing phase back to the start of the program, but
teach the stream-reading code never to override an existing value.
This is tricky, because stream "feature" lines override each other
(meaning we'd have to start tracking the source for every option).
3. Accept that we might parse a "feature export-marks" line that is
forbidden, as long we don't _act_ on it until after we've parsed
the command line options.
This would, in fact, work with the current code, but only because
the previous patch fixed the export-marks parser to avoid touching
the filesystem.
So while it works, it does carry risk of somebody getting it wrong
in the future in a rather subtle and unsafe way.
I've gone with option (1) here as simple, safe, and unlikely to cause
regressions.
This fixes CVE-2019-1348.
Signed-off-by: Jeff King <peff@peff.net>
When we parse the --export-marks option, we don't immediately open the
file, but we do create any leading directories. This can be especially
confusing when a command-line option overrides an in-stream one, in
which case we'd create the leading directory for the in-stream file,
even though we never actually write the file.
Let's instead create the directories just before opening the file, which
means we'll create only useful directories. Note that this could change
the handling of relative paths if we chdir() in between, but we don't
actually do so; the only permanent chdir is from setup_git_directory()
which runs before either code path (potentially we should take the
pre-setup dir into account to avoid surprising the user, but that's an
orthogonal change).
The test just adapts the existing "override" test to use paths with
leading directories. This checks both that the correct directory is
created (which worked before but was not tested), and that the
overridden one is not (our new fix here).
While we're here, let's also check the error result of
safe_create_leading_directories(). We'd presumably notice any failure
immediately after when we try to open the file itself, but we can give a
more specific error message in this case.
Signed-off-by: Jeff King <peff@peff.net>
Our tests confirm that providing two "import-marks" options in a
fast-import stream is an error. However, the invoked command would fail
even without covering this case, because the marks files themselves do
not actually exist. Let's create the files to make sure we fail for the
right reason (we actually do, because the option parsing happens before
we open anything, but this future-proofs our test).
Signed-off-by: Jeff King <peff@peff.net>
When the 'grep with invalid UTF-8 data' tests were added/adjusted in
8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26)
and 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching,
2019-07-26) they lacked a redirect which caused them to falsely succeed
on most systems. The 'grep -i' test failed on systems where JIT was
disabled as it never reached the portion which was missing the redirect.
A recent patch added the missing redirect and exposed the fact that the
'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' test fails
regardless of whether JIT is enabled.
Based on the final paragraph in in 870eea8166:
When grepping a non-ASCII fixed string. This is a more general problem
that's hard to fix, but we can at least fix the most common case of
grepping for a fixed string without "-i". I can't think of a reason
for why we'd turn on PCRE2_UTF when matching byte-for-byte like that.
it seems that we don't expect that the case-insensitive grep will
succeed. Adjust the test to reflect that expectation.
Signed-off-by: Todd Zullinger <tmz@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some codepaths in "gitweb" that forgot to escape URLs generated
based on end-user input have been corrected.
* jk/gitweb-anti-xss:
gitweb: escape URLs generated by href()
t/gitweb-lib.sh: set $REQUEST_URI
t/gitweb-lib.sh: drop confusing quotes
t9502: pass along all arguments in xss helper
"git fetch" codepath had a big "do not lazily fetch missing objects
when I ask if something exists" switch. This has been corrected by
marking the "does this thing exist?" calls with "if not please do not
lazily fetch it" flag.
* jt/fetch-remove-lazy-fetch-plugging:
promisor-remote: remove fetch_if_missing=0
clone: remove fetch_if_missing=0
fetch: remove fetch_if_missing=0
Recent update to "git stash pop" made the command empty the index
when run with the "--quiet" option, which has been corrected.
* tg/stash-refresh-index:
stash: make sure we have a valid index before writing it
Handling of commit objects that use non UTF-8 encoding during
"rebase -i" has been improved.
* dd/sequencer-utf8:
sequencer: reencode commit message for am/rebase --show-current-patch
sequencer: reencode old merge-commit message
sequencer: reencode squashing commit's message
sequencer: reencode revert/cherry-pick's todo list
sequencer: reencode to utf-8 before arrange rebase's todo list
t3900: demonstrate git-rebase problem with multi encoding
configure.ac: define ICONV_OMITS_BOM if necessary
t0028: eliminate non-standard usage of printf
The patterns to detect function boundary for Elixir language has
been added.
* ln/userdiff-elixir:
userdiff: add Elixir to supported userdiff languages
Docfix.
* en/doc-typofix:
Fix spelling errors in no-longer-updated-from-upstream modules
multimail: fix a few simple spelling errors
sha1dc: fix trivial comment spelling error
Fix spelling errors in test commands
Fix spelling errors in messages shown to users
Fix spelling errors in names of tests
Fix spelling errors in comments of testcases
Fix spelling errors in code comments
Fix spelling errors in documentation outside of Documentation/
Documentation: fix a bunch of typos, both old and new
The watchman integration for fsmonitor was racy, which has been
corrected to be more conservative.
* kw/fsmonitor-watchman-fix:
fsmonitor: fix watchman integration
"git worktree add" internally calls "reset --hard" that should not
descend into submodules, even when submodule.recurse configuration
is set, but it was affected. This has been corrected.
* pb/no-recursive-reset-hard-in-worktree-add:
worktree: teach "add" to ignore submodule.recurse config
"git rev-parse --git-path HEAD.lock" did not give the right path
when run in a secondary worktree.
* js/git-path-head-dot-lock-fix:
git_path(): handle `.lock` files correctly
t1400: wrap setup code in test case
The implementation of "git log --graph" got refactored and then its
output got simplified.
* jc/log-graph-simplify:
t4215: use helper function to check output
graph: fix coloring of octopus dashes
graph: flatten edges that fuse with their right neighbor
graph: smooth appearance of collapsing edges on commit lines
graph: rename `new_mapping` to `old_mapping`
graph: commit and post-merge lines for left-skewed merges
graph: tidy up display of left-skewed merges
graph: example of graph output that can be simplified
graph: extract logic for moving to GRAPH_PRE_COMMIT state
graph: remove `mapping_idx` and `graph_update_width()`
graph: reduce duplication in `graph_insert_into_new_columns()`
graph: reuse `find_new_column_by_commit()`
graph: handle line padding in `graph_next_line()`
graph: automatically track display width of graph lines
Crufty code and logic accumulated over time around the object
parsing and low-level object access used in "git fsck" have been
cleaned up.
* jk/cleanup-object-parsing-and-fsck: (23 commits)
fsck: accept an oid instead of a "struct tree" for fsck_tree()
fsck: accept an oid instead of a "struct commit" for fsck_commit()
fsck: accept an oid instead of a "struct tag" for fsck_tag()
fsck: rename vague "oid" local variables
fsck: don't require an object struct in verify_headers()
fsck: don't require an object struct for fsck_ident()
fsck: drop blob struct from fsck_finish()
fsck: accept an oid instead of a "struct blob" for fsck_blob()
fsck: don't require an object struct for report()
fsck: only require an oid for skiplist functions
fsck: only provide oid/type in fsck_error callback
fsck: don't require object structs for display functions
fsck: use oids rather than objects for object_name API
fsck_describe_object(): build on our get_object_name() primitive
fsck: unify object-name code
fsck: require an actual buffer for non-blobs
fsck: stop checking tag->tagged
fsck: stop checking commit->parent counts
fsck: stop checking commit->tree value
commit, tag: don't set parsed bit for parse failures
...
In this test, we have a command substitution whose output starts with a
NUL byte. bash and dash strip out any NUL bytes from the output; zsh
does not. As a consequence, zsh fails this test, since the command line
argument we use the variable in is truncated by the NUL byte.
POSIX says of a command substitution that if "the output contains any
null bytes, the behavior is unspecified," so all of the shells are in
compliance with POSIX. To make our code more portable, let's avoid
prefacing our variables with NUL bytes and instead leave only the
trailing one behind.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A number of t4210-log-i18n tests added in 4e2443b181 set LC_ALL to a UTF-8
locale (is_IS.UTF-8) but then pass an invalid UTF-8 string to --grep.
FreeBSD's regcomp() fails in this case with REG_ILLSEQ, "illegal byte
sequence," which git then passes to die():
fatal: command line: '�': illegal byte sequence
When these tests were added the commit message stated:
| It's possible that this
| test breaks the "basic" and "extended" backends on some systems that
| are more anal than glibc about the encoding of locale issues with
| POSIX functions that I can remember
which seems to be the case here.
Extend test-lib.sh to add a REGEX_ILLSEQ prereq, set it on FreeBSD, and
add !REGEX_ILLSEQ to the two affected tests.
Signed-off-by: Ed Maste <emaste@freebsd.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The file "empty" became unused with 1c5e94f459 (tests: use
'test_must_be_empty' instead of 'test_cmp <empty> <out>', 2018-08-19);
get rid of it.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The file "frontend" became unused with 4de0bbd898 (t9300: use perl
"head -c" clone in place of "dd bs=1 count=16000" kluge, 2010-12-13);
get rid of it.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since we're now recommending lore.kernel.org (and because the
public-inbox.org domain might eventually go away), let's update our
internal references to use it, too. That future-proofs our references,
and sets the example we want people to follow.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use test_must_be_empty instead of comparing it to an empty file. That's
more efficient, as the function only needs built-in meta-data only check
in the usual case, and provides nicer debug output otherwise.
Helped-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Two tests in t7812, added in 8a599983 ("grep: stess test PCRE v2 on
invalid UTF-8 data", 2019-07-26), were missing redirects, failing to
actually test the produced output.
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use test_must_be_empty instead of reading the file and comparing its
contents to an empty string. That's more efficient, as the function
only needs built-in meta-data only check in the usual case, and provides
nicer debug output otherwise.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use test_must_be_empty instead of reading the file and comparing its
contents to an empty string. That's more efficient, as the function
only needs built-in meta-data only check in the usual case, and provides
nicer debug output otherwise.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use test_line_count to check if the number of lines matches
expectations, for improved consistency and nicer debug output.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use test_line_count to check if the number of lines matches
expectations, for improved consistency and nicer debug output.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When writing a commit-graph, we show progress along several commit
walks. When we use start_delayed_progress(), the progress line will
only appear if that step takes a decent amount of time.
However, one place was missed: computing generation numbers. This is
normally a very fast operation as all commits have been parsed in a
previous step. But, this is showing up for all users no matter how few
commits are being added.
The tests that check for the progress output have already been updated
to use GIT_PROGRESS_DELAY=0 to force the expected output.
Helped-by: Jeff King <peff@peff.net>
Reported-by: ryenus <ryenus@gmail.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The start_delayed_progress() method is a preferred way to show
optional progress to users as it ignores steps that take less
than two seconds. However, this makes testing unreliable as tests
expect to be very fast.
In addition, users may want to decrease or increase this time
interval depending on their preferences for terminal noise.
Create the GIT_PROGRESS_DELAY environment variable to control
the delay set during start_delayed_progress(). Set the value
in some tests to guarantee their output remains consistent.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The perf suite's aggregate.perl depends on Git.pm, which is a mild
annoyance if you've built git with NO_PERL. It turns out that the only
thing we use it for is a single call of the command_oneline() helper.
We can just replace this with backticks or similar.
Annoyingly, perl has no backtick equivalent that avoids a shell eval,
which means our $arg would require quoting. This probably doesn't matter
for our purposes, but it's better to be safe and model good style. So
we'll just provide a short helper around open(), which takes its
arguments as a list.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The perf tests write files recording the results of tests. These
results are later aggregated by 'aggregate.perl'. If the tests are run
multiple times, those results are overwritten by the new results. This
works just fine as long as there are only perf tests measuring the
times, whose results are stored in "$base".times files.
However 22bec79d1a ("t/perf: add infrastructure for measuring sizes",
2018-08-17) introduced a new type of test for measuring the size of
something. The results of this are written to "$base".size files.
"$base" is essentially made up of the basename of the script plus the
test number. So if test numbers shift because a new test was
introduced earlier in the script we might end up with both a ".times"
and a ".size" file for the same test. In the aggregation script the
".times" file is preferred over the ".size" file, so some size tests
might end with performance numbers from a previous run of the test.
This is mainly relevant when writing perf tests that check both
performance and sizes, and can get quite confusing during
developement.
We could fix this by doing a more thorough job of cleaning out old
".times" and ".size" files before running each test. However, an even
easier solution is to just use the same filename for both types of
measurement, meaning we'll always overwrite the previous result. We
don't even need to change the file format to distinguish the two;
aggregate.perl already decides which is which based on a regex of the
content (this may become ambiguous if we add new types in the future,
but we could easily add a header field to the file at that point).
Based on an initial patch from Thomas Gummerer, who discovered the
problem and did all of the analysis (which I stole for the commit
message above):
https://public-inbox.org/git/20191119185047.8550-1-t.gummerer@gmail.com/
Helped-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When 'test_commit_bulk' is invoked in an empty test repository, it
prints a "fatal: Needed a single revision" error, but still does what
it's supposed to do. A test helper function displaying a fatal error
and still succeeding is always suspect to be buggy, but luckily that's
not the case here: that error comes from a 'git rev-parse --verify
HEAD' command invoked in a condition, which doesn't have anything to
verify in an empty repository.
Use the '--quiet' option to suppress that error message.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When calling `git submodule status` while in a subdirectory, we are
incorrectly not detecting modified submodules and
thus reporting that all of the submodules are unchanged.
This is because the submodule helper is calling `diff-index` with the
submodule path assuming the path is relative to the current prefix
directory, however the submodule path used is actually relative to the root.
Always pass NULL as the `prefix` when running diff-files on the
submodule, to make sure the submodule's path is interpreted as relative
to the superproject's repository root.
Signed-off-by: Manish Goregaokar <manishsmail@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since 2f328c3d ("reset $sha1 $pathspec: require $sha1 only to be
treeish", 2013-01-14), we allowed "git reset $object -- $path" to reset
individual paths that match the pathspec to take the blob from a tree
object, not necessarily a commit, while the form to reset the tip of the
current branch to some other commit still must be given a commit.
Like resetting with paths, "git reset --patch" does not update HEAD, and
need not require a commit. The path-filtered form, "git reset --patch
$object -- $pathspec", has accepted a tree-ish since 2f328c3d.
"git reset --patch" is documented as accepting a <tree-ish> since
bf44142f ("reset: update documentation to require only tree-ish with
paths", 2013-01-16). Documentation changes are not required.
Loosen the restriction that requires a commit for the unfiltered "git
reset --patch $object".
Signed-off-by: Nika Layzell <nika@thelayzells.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When 'git revert' or 'git cherry-pick --edit' is invoked with multiple
commits, then after editing the first commit message is finished both
these commands should continue with processing the second commit and
launch another editor for its commit message, assuming there are
no conflicts, of course.
Alas, this inadvertently changed with commit a47ba3c777 (rebase -i:
check for updated todo after squash and reword, 2019-08-19): after
editing the first commit message is finished, both 'git revert' and
'git cherry-pick --edit' exit with error, claiming that "nothing to
commit, working tree clean".
The reason for the changed behaviour is twofold:
- Prior to a47ba3c777 the up-to-dateness of the todo list file was
only checked after 'exec' instructions, and that commit moved
those checks to the common code path. The intention was that this
check should be performed after instructions spawning an editor
('squash' and 'reword') as well, so the ongoing 'rebase -i'
notices when the user runs a 'git rebase --edit-todo' while
squashing/rewording a commit message.
However, as it happened that check is now performed even after
'revert' and 'pick' instructions when they involved editing the
commit message. And 'revert' by default while 'pick' optionally
(with 'git cherry-pick --edit') involves editing the commit
message.
- When invoking 'git revert' or 'git cherry-pick --edit' with
multiple commits they don't read a todo list file but assemble the
todo list in memory, thus the associated stat data used to check
whether the file has been updated is all zeroed out initially.
Then the sequencer writes all instructions (including the very
first) to the todo file, executes the first 'revert/pick'
instruction, and after the user finished editing the commit
message the changes of a47ba3c777 kick in, and it checks whether
the todo file has been modified. The initial all-zero stat data
obviously differs from the todo file's current stat data, so the
sequencer concludes that the file has been modified. Technically
it is not wrong, of course, because the file just has been written
indeed by the sequencer itself, though the file's contents still
match what the sequencer was invoked with in the beginning.
Consequently, after re-reading the todo file the sequencer
executes the same first instruction _again_, thus ending up in
that "nothing to commit" situation.
The todo list was never meant to be edited during multi-commit 'git
revert' or 'cherry-pick' operations, so perform that "has the todo
file been modified" check only when the sequencer was invoked as part
of an interactive rebase.
Reported-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
By default, CreateProcess() does not inherit any open file handles,
unless the bInheritHandles parameter is set to TRUE. Which we do need to
set because we need to pass in stdin/stdout/stderr to talk to the child
processes. Sadly, this means that all file handles (unless marked via
O_NOINHERIT) are inherited.
This lead to problems in VFS for Git, where a long-running read-object
hook is used to hydrate missing objects, and depending on the
circumstances, might only be called *after* Git opened a file handle.
Ideally, we would not open files without O_NOINHERIT unless *really*
necessary (i.e. when we want to pass the opened file handle as standard
handle into a child process), but apparently it is all-too-easy to
introduce incorrect open() calls: this happened, and prevented updating
a file after the read-object hook was started because the hook still
held a handle on said file.
Happily, there is a solution: as described in the "Old New Thing"
https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873 there
is a way, starting with Windows Vista, that lets us define precisely
which handles should be inherited by the child process.
And since we bumped the minimum Windows version for use with Git for
Windows to Vista with v2.10.1 (i.e. a *long* time ago), we can use this
method. So let's do exactly that.
We need to make sure that the list of handles to inherit does not
contain duplicates; Otherwise CreateProcessW() would fail with
ERROR_INVALID_ARGUMENT.
While at it, stop setting errno to ENOENT unless it really is the
correct value.
Also, fall back to not limiting handle inheritance under certain error
conditions (e.g. on Windows 7, which is a lot stricter in what handles
you can specify to limit to).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When spawning child processes, we really should be careful which file
handles we let them inherit.
This is doubly important on Windows, where we cannot rename, delete, or
modify files if there is still a file handle open.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The GIT_TEST_CLONE_2GB environment variable is only ever checked with
'test -z' in 't5608-clone-2gb.sh', so any non-empty value is
interpreted as "yes, run these expensive tests", even
'GIT_TEST_CLONE_2GB=NoThanks'.
Similar GIT_TEST_* environment variables have already been turned into
bools in 3b072c577b (tests: replace test_tristate with "git
env--helper", 2019-06-21), so let's turn GIT_TEST_CLONE_2GB into a
bool as well, to follow suit.
Our CI builds set GIT_TEST_CLONE_2GB=YesPlease, so adjust them
accordingly, thus removing the last 'YesPlease' from our CI scripts.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since 3b072c577b (tests: replace test_tristate with "git env--helper",
2019-06-21) we get the normalized bool values of various GIT_TEST_*
environment variables via 'git env--helper'. Now, while the 'git
env--helper' command itself does catch invalid values in the
environment variable or in the given --default and exits with error
(exit code 128 or 129, respectively), it's invoked in conditions like
'if ! git env--helper ...', which means that all invalid bool values
are interpreted the same as the ordinary 'false' (exit code 1). This
has led to inadvertently skipped httpd tests in our CI builds for a
couple of weeks, see 3960290675 (ci: restore running httpd tests,
2019-09-06).
Let's be more careful about what the test suite accepts as bool values
in GIT_TEST_* environment variables, and error out loud and clear on
invalid values instead of simply skipping tests. Add the
'test_bool_env' helper function to encapsulate the invocation of 'git
env--helper' and the verification of its exit code, and replace all
invocations of that command in our test framework and test suite with
a call to this new helper (except in 't0017-env-helper.sh', of
course).
$ GIT_TEST_GIT_DAEMON=YesPlease ./t5570-git-daemon.sh
fatal: bad numeric config value 'YesPlease' for 'GIT_TEST_GIT_DAEMON': invalid unit
error: test_bool_env requires bool values both for $GIT_TEST_GIT_DAEMON and for the default fallback
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This fixes a regression introduced in 356ee4659b ("sequencer: try to
commit without forking 'git commit'", 2017-11-24). When amending a
commit try_to_commit() was using the wrong parent when checking if the
commit would be empty. When amending we need to check against HEAD^ not
HEAD.
t3403 may not seem like the natural home for the new tests but a further
patch series will improve the advice printed by `git commit`. That
series will mutate these tests to check that the advice includes
suggesting `rebase --skip` to skip the fixup that would empty the
commit.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The VALIDSIG status line from GnuPG with --status-fd is documented to
have 9 required and 1 optional fields [1]. The final, and optional,
field is used to specify the fingerprint of the primary key that made
the signature in case it was made by a subkey. However, this field is
only available for OpenPGP signatures; not for CMS/X.509.
If the VALIDSIG status line does not have the optional 10th field, the
current code will continue reading onto the next status line. And this
is the case for non-OpenPGP signatures [1].
The consequence is that a subsequent status line may be considered as
the "primary key" for signatures that does not have an actual primary
key.
Limit the search of these 9 or 10 fields to the single line to avoid
this problem. If the 10th field is missing, report that there is no
primary key fingerprint.
[Reference]
[1] GnuPG Details, General status codes
https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=6ce340e8c04794add995e84308bb3091450bd28f;hb=HEAD#l483
The documentation says:
VALIDSIG <args>
The args are:
- <fingerprint_in_hex>
- <sig_creation_date>
- <sig-timestamp>
- <expire-timestamp>
- <sig-version>
- <reserved>
- <pubkey-algo>
- <hash-algo>
- <sig-class>
- [ <primary-key-fpr> ]
This status indicates that the signature is cryptographically
valid. [...] PRIMARY-KEY-FPR is the fingerprint of the primary key
or identical to the first argument.
The primary-key-fpr parameter is used for OpenPGP and not available
for CMS signatures. [...]
Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The index might be aware that a file hasn't modified via fsmonitor, but
unpack-trees did not pay attention to it and checked via ie_match_stat
which can be inefficient on certain filesystems. This significantly slows
down commands that run oneway_merge, like checkout and reset --hard.
This patch makes oneway_merge check whether a file is considered
unchanged through fsmonitor and skips ie_match_stat on it. unpack-trees
also now correctly copies over fsmonitor validity state from the source
index. Finally, for correctness, we force a refresh of fsmonitor state in
tweak_fsmonitor.
After this change, commands like stash (that use reset --hard
internally) go from 8s or more to ~2s on a 250k file repository on a
mac.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Kevin Willford <Kevin.Willford@microsoft.com>
Signed-off-by: Utsav Shah <utsav@dropbox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently, if a git command fails in an unexpected way, such as a
segfault, it will be masked and ignored. Replace the ! with
test_must_fail so that only expected failures pass.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the previous patches, the mechanical application of changes left some
duplicate statements in the test case which were not strictly incorrect
but were redundant and possibly misleading. Remove these duplicate
statements so that it is clear that the intent behind the tests are that
the content of the file stays the same throughout the whole test case.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We currently have many instances of `test <line> = $(cat <file>)` and
`test $(cat <file>) = <line>`. In the case where this fails, it will be
difficult for a developer to debug since the output will be masked.
Replace these instances with invocations of test_cmp().
This change was done with the following GNU sed expressions:
s/\(\s*\)test \([^=]*\)= "$(cat \([^)]*\))"/\1echo \2>expect \&\&\n\1test_cmp expect \3/
s/\(\s*\)test "$(cat \([^)]*\))" = \([^&]*\)\( &&\)\?$/\1echo \3 >expect \&\&\n\1test_cmp expect \2\4/
A future patch will clean up situations where we have multiple duplicate
statements within a test case. This is done to keep this patch purely
mechanical.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Before, if the invocation of git failed, it would be masked by the pipe
since only the return code of the last element of a pipe is used.
Rewrite the test to put the git command on its own line so its return
code is not masked.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In case an invocation of a git command fails within the command
substitution, the failure will be masked. Replace the command
substitution with a file-redirection and a call to test_cmp.
This change was done with the following GNU sed expressions:
s/\(\s*\)test \([^ ]*\) = "$(\(git [^)]*\))"/\1echo \2 >expect \&\&\n\1\3 >actual \&\&\n\1test_cmp expect actual/
s/\(\s*\)test "$(\(git [^)]*\))" = \([^ ]*\)/\1echo \3 >expect \&\&\n\1\2 >actual \&\&\n\1test_cmp expect actual/
A future patch will clean up situations where we have multiple duplicate
statements within a test case. This is done to keep this patch purely
mechanical.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In case an invocation of `git rev-list` fails within the command
substitution, the failure will be masked. Remove the command
substitution and use test_cmp_rev() so that failures can be discovered.
This change was done with the following sed expressions:
s/test "$(git rev-parse.* \([^)]*\))" = "$(git rev-parse \([^)]*\))"/test_cmp_rev \1 \2/
s/test \([^ ]*\) = "$(git rev-parse.* \([^)]*\))"/test_cmp_rev \1 \2/
s/test "$(git rev-parse.* \([^)]*\))" != "$(git rev-parse.* \([^)]*\))"/test_cmp_rev ! \1 \2/
s/test \([^ ]*\) != "$(git rev-parse.* \([^)]*\))"/test_cmp_rev ! \1 \2/
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When wrapping a git command in a command substitution within another
command, we throw away the git command's exit code. In case the git
command fails, we would like to know about it rather than the failure
being silent. Extract git commands so that their exit codes are not
lost.
Instead of using `test -n` or `test -z`, replace them respectively with
invocations of test_file_not_empty() and test_must_be_empty() so that we
get better debugging information in the case of a failure.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Instead of rolling our own functionality to test the number of lines a
command outputs, use test_line_count() which provides better debugging
information in the case of a failure.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The style for tests in Git is to have the redirect operator attached to
the filename with no spaces. Fix test cases where this is not the case.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Although `test -f` has the same functionality as test_path_is_file(), in
the case where test_path_is_file() fails, we get much better debugging
information.
Replace `test -f` with test_path_is_file() so that future developers
will have a better experience debugging these test cases.
Also, in the case of `! test -f`, not only should that path not be a
file, it shouldn't exist at all so replace it with
test_path_is_missing().
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We were using a redirection operator to feed input into sed. However,
since sed is capable of opening its own files, make sed open its own
files instead of redirecting input into it.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The usual convention is for test case names to be written between
single-quotes. Change all double-quoted test case names to single-quotes
except for two test case names that use variables within.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Improve the test style by removing leading and trailing empty lines
within test cases. Also, reformat multi-line subshells to conform to the
existing style.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the case where we are using test_cmp_rev() to report not-equals, we
write `! test_cmp_rev`. However, since test_cmp_rev() contains
r1=$(git rev-parse --verify "$1") &&
r2=$(git rev-parse --verify "$2") &&
`! test_cmp_rev` will succeed if any of the rev-parses fail. This
behavior is not desired. We want the rev-parses to _always_ be
successful.
Rewrite test_cmp_rev() to optionally accept "!" as the first argument to
do a not-equals comparison. Rewrite `! test_cmp_rev` to `test_cmp_rev !`
in all tests to take advantage of this new functionality.
Also, rewrite the rev-parse logic to end with a `|| return 1` instead of
&&-chaining into the rev-comparison logic. This makes it obvious to
future readers that we explicitly intend on returning early if either of
the rev-parses fail.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
According to POSIX enhancement request '0000767: Add built-in
"local"'[1],
dash only allows one variable in a local definition; it permits
assignment though it doesn't document that clearly.
however, this isn't true since t0000 still passes with this patch
applied on dash 0.5.10.2. Needless to say, since `local` isn't POSIX
standardized, it is not exactly clear what `local` entails on different
versions of different shells.
We currently already have many instances of multiple local assignments
in our codebase. Ensure that this is actually supported by explicitly
testing that it is sane.
[1]: http://austingroupbugs.net/bug_view_page.php?bug_id=767
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Since format-patch accepts `--[no-]notes`, one would expect the
range-diff generated to also respect the setting. Unfortunately, the
range-diff we currently generate only uses the default option (which
always outputs default notes, even when notes are not being used
elsewhere).
Pass the notes configuration to range-diff so that it can honor it.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a commit being range-diff'd has a note attached to it, the note
will be compared as well. However, if a user has multiple notes refs or
if they want to suppress notes from being printed, there is currently no
way to do this.
Pass through `--[no-]notes[=<ref>]` to the `git log` call so that this
option is customizable.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When notes were included in the output of range-diff, they were just
mashed together with the rest of the commit message. As a result, users
wouldn't be able to clearly distinguish where the commit message ended
and where the notes started.
Output a `## Notes ##` header when notes are detected so that notes can
be compared more clearly.
Note that we handle case of `Notes (<ref>): -> ## Notes (<ref>) ##` with
this code as well. We can't test this in this patch, however, since
there is currently no way to pass along different notes refs to `git
log`. This will be fixed in a future patch.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The test suite had a blindspot where it did not check the behavior of
range-diff and format-patch when notes were present. Cover this
blindspot.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For test cases, the usual convention is to name expected output files
"expect", not "expected". Replace all instances of "expected" with
"expect".
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the first heredoc, parameter substitution is not used so prevent it
from happening in the future (perhaps by accident) by escaping the limit
EOF.
The remaining heredocs use parameter substitution so they cannot be
changed.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For shell scripts, the usual convention is for there to be no space
after redirection operators, (e.g. `>file`, not `> file`). Remove the
one instance of this.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Python's async functions (declared with "async def" rather than "def")
were not being displayed in hunk headers. This commit teaches git about
the async function syntax, and adds tests for the Python userdiff regex.
Signed-off-by: Josh Holland <anowlcalledjosh@gmail.com>
Acked-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The standard format for referencing other commits within some projects
(such as git.git) is the reference format. This is described in
Documentation/SubmittingPatches as
If you want to reference a previous commit in the history of a stable
branch, use the format "abbreviated hash (subject, date)", like this:
....
Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
noticed that ...
....
Since this format is so commonly used, standardize it as a pretty
format.
The tests that are implemented essentially show that the format-string
does not change in response to various log options. This is useful
because, for future developers, it shows that we've considered the
limitations of the "canned format-string" approach and we are fine with
them.
Based-on-a-patch-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add the placeholders %as and %cs to format author date and committer
date, respectively, without the time part, like --date=short does, i.e.
like YYYY-MM-DD.
Signed-off-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The test suite does not include any tests where `--reflog` and `-z` are
used together in `git log`. Cover this blindspot. Note that the
`--pretty=oneline` case is written separately because it follows a
slightly different codepath.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Decisions taken for simplicity:
1) For now, `--pathspec-from-file` is declared incompatible with
`--interactive/--patch`, even when <file> is not `stdin`. Such use
case it not really expected. Also, it would require changes to
`interactive_add()`.
2) It is not allowed to pass pathspec in both args and file.
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Decisions taken for simplicity:
1) For now, `--pathspec-from-file` is declared incompatible with
`--patch`, even when <file> is not `stdin`. Such use case it not
really expected. Also, it is harder to support in `git commit`, so
I decided to make it incompatible in all places.
2) It is not allowed to pass pathspec in both args and file.
Co-authored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In order to work correctly, git-rebase --rebase-merges needs to make
initial todo list with unique labels.
Those unique labels is being handled by employing a hashmap and
appending an unique number if any duplicate is found.
But, we forget that beside those labels for side branches,
we also have a special label `onto' for our so-called new-base.
In a special case that any of those labels for side branches named
`onto', git will run into trouble.
Correct it.
Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ever since it was introduced in 7cceca5ccc (Add 'git rev-parse
--show-toplevel' option., 2010-01-12), the --show-toplevel option has
treated a missing working tree as a quiet success: it neither prints a
toplevel path, but nor does it report any kind of error.
While a caller could distinguish this case by looking for an empty
response, the behavior is rather confusing. We're better off complaining
that there is no working tree, as other internal commands would do in
similar cases (e.g., "git status" or any builtin with NEED_WORK_TREE set
would just die()). So let's do the same here.
While we're at it, let's clarify the documentation and add some tests,
both for the new behavior and for the more mundane case (which was not
covered).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `label` todo command in interactive rebases creates temporary refs
in the `refs/rewritten/` namespace. These refs are stored as loose refs,
i.e. as files in `.git/refs/rewritten/`, therefore they have to conform
with file name limitations on the current filesystem in addition to the
accepted ref format.
This poses a problem in particular on NTFS/FAT, where e.g. the colon,
double-quote and pipe characters are disallowed as part of a file name.
Let's safeguard against this by replacing not only white-space
characters by dashes, but all non-alpha-numeric ones.
However, we exempt non-ASCII UTF-8 characters from that, as it should be
quite possible to reflect branch names such as `↯↯↯` in refs/file names.
Signed-off-by: Matthew Rogers <mattr94@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This imitates the code to show the help text from the Perl script
`git-add--interactive.perl` in the built-in version.
To make sure that it renders exactly like the Perl version of `git add
-i`, we also add a test case for that to `t3701-add-interactive.sh`.
Signed-off-by: Slavica Đukić <slawica92@hotmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
There's a cross-site scripting problem in gitweb, where it will print
URLs generated by its href() helper without further quoting. This allows
an attacker to point a victim to a specially crafted gitweb URL and
inject arbitrary HTML into the resulting page (which the victim sees as
coming from gitweb).
The base of the URL comes from evaluate_uri(), which pulls the value of
$REQUEST_URI via the CGI module. It tries to strip off $PATH_INFO, but
fails to do so in some cases (including ones that contain special
characters, like "+"). Most of the uses of the URL end up being passed
to "$cgi->a(-href = href())", which will get quoted properly by the CGI
module. But in a few places, we output them ourselves as part of
manually-generated HTML, and whatever was in the original URL will
appear unquoted in the output.
Given that all of the nearby variables placed into this manual HTML
_are_ quoted, it seems like the authors assumed that these URLs would
not need quoting. So it's possible that the bug is actually in
evaluate_uri(), which should be doing a more careful job of stripping
$PATH_INFO. There's some discussion in a comment in that function, as
well as the commit message in 81d3fe9f48 (gitweb: fix wrong base URL
when non-root DirectoryIndex, 2009-02-15). But I'm not sure I understand
it.
Regardless, it's a good idea to quote these values at the point of
insertion into the HTML output:
1. Even if there is a bug in evaluate_uri(), this would give us
belt-and-suspenders protection.
2. evaluate_uri() is only handling the base. Some generated URLs will
also mention arbitrary refs or filenames in the repositories, and
these should be quoted anyway.
3. It should never _hurt_ to quote (and that's what all of the
$cgi->a() calls are doing already).
So there may be further work here, but this patch at least prevents the
XSS vulnerability, and shouldn't make anything worse.
The test here covers the calls in print_feed_meta(), but I manually
audited every call to href() to see how its output was used, and quoted
appropriately. Most of them are esc_attr(), as they're used in tag
attributes, but I used esc_html() when the URLs were printed bare. The
distinction is largely academic, as one is implemented as a wrapper for
the other.
Reported-by: NAKAYAMA DAISUKE <nakyamad@icloud.com>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a real webserver's CGI call, gitweb.cgi would typically see
$REQUEST_URI set. This variable does impact how we display our URL in
the resulting page, so let's try to make our test as realistic as
possible (we can just use the $PATH_INFO our caller passed in, if any).
This doesn't change the outcome of any tests, but it will help us add
some new tests in a future patch.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Some variables assignments in gitweb_run() look like this:
FOO=""$1""
The extra quotes aren't doing anything. Each set opens and closes an
empty string, and $1 is actually outside of any double-quotes (which is
OK, because variable assignment does not do whitespace splitting on the
expanded value).
Let's drop them, as they're simply confusing.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This function is just a thin wrapper around gitweb_run(), which takes
multiple arguments. But we only pass along "$1". Let's pass everything
we get, which will let a future patch add an XSS test that affects
PATH_INFO (which gitweb_run() takes as $2).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We try to delete the non-existing tag "anothertag", but for the
verifications, we check that the tag "myhead" doesn't exist. "myhead"
isn't used in this test except for this checking. Comparing to the test
two tests earlier, it looks like a copy-paste mistake.
Perhaps it's overkill to check that `git tag -d` didn't decide to
*create* a tag. But since we're trying to be this careful, let's
actually check the correct tag. While we're doing this, let's use a more
descriptive tag name instead -- "nonexistingtag" should be obvious.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Unlike previous conversions to C, where we started with a built-in
helper, we start this conversion by adding an interception in the
`run_add_interactive()` function when the new opt-in
`add.interactive.useBuiltin` config knob is turned on (or the
corresponding environment variable `GIT_TEST_ADD_I_USE_BUILTIN`), and
calling the new internal API function `run_add_i()` that is implemented
directly in libgit.a.
At this point, the built-in version of `git add -i` only states that it
cannot do anything yet. In subsequent patches/patch series, the
`run_add_i()` function will gain more and more functionality, until it
is feature complete. The whole arc of the conversion can be found in the
PRs #170-175 at https://github.com/gitgitgadget/git.
The "--helper approach" can unfortunately not be used here: on Windows
we face the very specific problem that a `system()` call in
Perl seems to close `stdin` in the parent process when the spawned
process consumes even one character from `stdin`. Which prevents us from
implementing the main loop in C and still trying to hand off to the Perl
script.
The very real downside of the approach we have to take here is that the
test suite won't pass with `GIT_TEST_ADD_I_USE_BUILTIN=true` until the
conversion is complete (the `--helper` approach would have let it pass,
even at each of the incremental conversion steps).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
