Skip to content

Conversation

@garimasi514
Copy link
Owner

Thanks for taking the time to contribute to Git!

Those seeking to contribute to the Git for Windows fork should see
http://gitforwindows.org/#contribute on how to contribute Windows specific
enhancements.

If your contribution is for the core Git functions and documentation
please be aware that the Git community does not use the github.com issues
or pull request mechanism for their contributions.

Instead, we use the Git mailing list ([email protected]) for code and
documenatation submissions, code reviews, and bug reports. The
mailing list is plain text only (anything with HTML is sent directly
to the spam folder).

Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!

dscho and others added 30 commits August 16, 2019 23:53
We are about to introduce the function `enable_non_canonical()`, which
shares almost the complete code with `disable_echo()`.

Let's prepare for that, by refactoring out that shared code.

Signed-off-by: Johannes Schindelin <[email protected]>
Git for Windows' Git Bash runs in MinTTY by default, which does not have
a Win32 Console instance, but uses MSYS2 pseudo terminals instead.

This is a problem, as Git for Windows does not want to use the MSYS2
emulation layer for Git itself, and therefore has no direct way to
interact with that pseudo terminal.

As a workaround, use the `stty` utility (which is included in Git for
Windows, and which *is* an MSYS2 program, so it knows how to deal with
the pseudo terminal).

Note: If Git runs in a regular CMD or PowerShell window, there *is* a
regular Win32 Console to work with. This is not a problem for the MSYS2
`stty`: it copes with this scenario just fine.

Also note that we introduce support for more bits than would be
necessary for a mere `disable_echo()` here, in preparation for the
upcoming `enable_non_canonical()` function.

Signed-off-by: Johannes Schindelin <[email protected]>
Typically, input on the command-line is line-based. It is actually not
really easy to get single characters (or better put: keystrokes).

We provide two implementations here:

- One that handles `/dev/tty` based systems as well as native Windows.
  The former uses the `tcsetattr()` function to put the terminal into
  "raw mode", which allows us to read individual keystrokes, one by one.
  The latter uses `stty.exe` to do the same, falling back to direct
  Win32 Console access.

  Thanks to the refactoring leading up to this commit, this is a single
  function, with the platform-specific details hidden away in
  conditionally-compiled code blocks.

- A fall-back which simply punts and reads back an entire line.

Note that the function writes the keystroke into an `strbuf` rather than
a `char`, in preparation for reading Escape sequences (e.g. when the
user hit an arrow key). This is also required for UTF-8 sequences in
case the keystroke corresponds to a non-ASCII letter.

Signed-off-by: Johannes Schindelin <[email protected]>
The scripted version of `git stash` called directly into the Perl script
`git-add--interactive.perl`, and this was faithfully converted to C.

However, we have a much better way to do this now: call `git add
--patch=<mode>`, which incidentally also respects the config setting
`add.interactive.useBuiltin`.

Let's do this.

Signed-off-by: Johannes Schindelin <[email protected]>
The Perl version of `git add -p` supports this config setting to allow
users to input commands via single characters (as opposed to having to
press the <Enter> key afterwards).

This is an opt-in feature because it requires Perl packages
(Term::ReadKey and Term::Cap, where it tries to handle an absence of the
latter package gracefully) to work. Note that at least on Ubuntu, that
Perl package is not installed by default (it needs to be installed via
`sudo apt-get install libterm-readkey-perl`), so this feature is
probably not used a whole lot.

In C, we obviously do not have these packages available, but we just
introduced `read_single_keystroke()` that is similar to what
Term::ReadKey provides, and we use that here.

Signed-off-by: Johannes Schindelin <[email protected]>
This recapitulates part of b5cc003 (add -i: ignore terminal escape
sequences, 2011-05-17):

    add -i: ignore terminal escape sequences

    On the author's terminal, the up-arrow input sequence is ^[[A, and
    thus fat-fingering an up-arrow into 'git checkout -p' is quite
    dangerous: git-add--interactive.perl will ignore the ^[ and [
    characters and happily treat A as "discard everything".

    As a band-aid fix, use Term::Cap to get all terminal capabilities.
    Then use the heuristic that any capability value that starts with ^[
    (i.e., \e in perl) must be a key input sequence.  Finally, given an
    input that starts with ^[, read more characters until we have read a
    full escape sequence, then return that to the caller.  We use a
    timeout of 0.5 seconds on the subsequent reads to avoid getting stuck
    if the user actually input a lone ^[.

    Since none of the currently recognized keys start with ^[, the net
    result is that the sequence as a whole will be ignored and the help
    displayed.

Note that we leave part for later which uses "Term::Cap to get all
terminal capabilities", for several reasons:

1. it is actually not really necessary, as the timeout of 0.5 seconds
   should be plenty sufficient to catch Escape sequences,

2. it is cleaner to keep the change to special-case Escape sequences
   separate from the change that reads all terminal capabilities to
   speed things up, and

3. in practice, relying on the terminal capabilities is a bit overrated,
   as the information could be incomplete, or plain wrong. For example,
   in this developer's tmux sessions, the terminal capabilities claim
   that the "cursor up" sequence is ^[M, but the actual sequence
   produced by the "cursor up" key is ^[[A.

Signed-off-by: Johannes Schindelin <[email protected]>
When `interactive.singlekey = true`, we react immediately to keystrokes,
even to Escape sequences (e.g. when pressing a cursor key).

The problem with Escape sequences is that we do not really know when
they are done, and as a heuristic we poll standard input for half a
second to make sure that we got all of it.

While waiting half a second is not asking for a whole lot, it can become
quite annoying over time, therefore with this patch, we read the
terminal capabilities (if available) and extract known Escape sequences
from there, then stop polling immediately when we detected that the user
pressed a key that generated such a known sequence.

This recapitulates the remaining part of b5cc003 (add -i: ignore
terminal escape sequences, 2011-05-17).

Note: We do *not* query the terminal capabilities directly. That would
either require a lot of platform-specific code, or it would require
linking to a library such as ncurses.

Linking to a library in the built-ins is something we try very hard to
avoid (we even kicked the libcurl dependency to a non-built-in remote
helper, just to shave off a tiny fraction of a second from Git's startup
time). And the platform-specific code would be a maintenance nightmare.

Even worse: in Git for Windows' case, we would need to query MSYS2
pseudo terminals, which `git.exe` simply cannot do (because it is
intentionally *not* an MSYS2 program).

To address this, we simply spawn `infocmp -L -1` and parse its output
(which works even in Git for Windows, because that helper is included in
the end-user facing installations).

This is done only once, as in the Perl version, but it is done only when
the first Escape sequence is encountered, not upon startup of `git add
-i`; This saves on startup time, yet makes reacting to the first Escape
sequence slightly more sluggish. But it allows us to keep the
terminal-related code encapsulated in the `compat/terminal.c` file.

Signed-off-by: Johannes Schindelin <[email protected]>
In 7e9e048 (stash -p: demonstrate failure of split with mixed y/n,
2015-04-16), a regression test for a known breakage that was added to
the test script `t3904-stash-patch.sh` that demonstrated that splitting
a hunk and trying to stash only part of that split hunk fails (but
shouldn't).

As expected, it still fails, but for the wrong reason: once the bug is
fixed, we would expect stderr to show nothing, yet the regression test
expects stderr to show something.

Let's fix that by telling that regression test case to expect nothing to
be printed to stderr.

While at it, also drop the obvious left-over from debugging where the
regression test did not mind `git stash -p` to return a non-zero exit
status.

Of course, the regression test still fails, but this time for the
correct reason.

Signed-off-by: Johannes Schindelin <[email protected]>
This job runs the test suite twice, once in regular mode, and once with
a whole slew of `GIT_TEST_*` variables set.

Now that the built-in version of `git add --interactive` is
feature-complete, let's also throw `GIT_TEST_MULTI_PACK_INDEX` into that
fray.

Signed-off-by: Johannes Schindelin <[email protected]>
When trying to stash part of the worktree changes by splitting a hunk
and then only partially accepting the split bits and pieces, the user
is presented with a rather cryptic error:

	error: patch failed: <file>:<line>
	error: test: patch does not apply
	Cannot remove worktree changes

and the command would fail to stash the desired parts of the worktree
changes (even if the `stash` ref was actually updated correctly).

We even have a test case demonstrating that failure, carrying it for
four years already.

The explanation: when splitting a hunk, the changed lines are no longer
separated by more than 3 lines (which is the amount of context lines
Git's diffs use by default), but less than that. So when staging only
part of the diff hunk for stashing, the resulting diff that we want to
apply to the worktree in reverse will contain those changes to be
dropped surrounded by three context lines, but since the diff is
relative to HEAD rather than to the worktree, these context lines will
not match.

Example time. Let's assume that the file README contains these lines:

	We
	the
	people

and the worktree added some lines so that it contains these lines
instead:

	We
	are
	the
	kind
	people

and the user tries to stash the line containing "are", then the command
will internally stage this line to a temporary index file and try to
revert the diff between HEAD and that index file. The diff hunk that
`git stash` tries to revert will look somewhat like this:

	@@ -1776,3 +1776,4
	 We
	+are
	 the
	 people

It is obvious, now, that the trailing context lines overlap with the
part of the original diff hunk that the user did *not* want to stash.

Keeping in mind that context lines in diffs serve the primary purpose of
finding the exact location when the diff does not apply precisely (but
when the exact line number in the file to be patched differs from the
line number indicated in the diff), we work around this by reducing the
amount of context lines: the diff was just generated.

Note: this is not a *full* fix for the issue. Just as demonstrated in
t3701's 'add -p works with pathological context lines' test case, there
are ambiguities in the diff format. It is very rare in practice, of
course, to encounter such repeated lines.

The full solution for such cases would be to replace the approach of
generating a diff from the stash and then applying it in reverse by
emulating `git revert` (i.e. doing a 3-way merge). However, in `git
stash -p` it would not apply to `HEAD` but instead to the worktree,
which makes this non-trivial to implement as long as we also maintain a
scripted version of `add -i`.

Signed-off-by: Johannes Schindelin <[email protected]>
The first part of the long journey to a fully built-in `git add -i`.

It reflects the part that was submitted a couple of times (see
gitgitgadget#103) during the Outreachy
project by Slavica Đukić that continued the journey based on an initial
patch series by Daniel Ferreira.

This part only implements the `status` and the `help` part, like
Slavica's last iteration did, in the interest of making the review
remotely more reviewable. I fear that this attempt of making it a bit
more reviewable is pretty futile, as so many things changed. So I will
ask the reviewers for forgiveness: please be kind, and give this sort of
a fresh review.

I threw in a couple of major changes on top of that iteration, though:

- The original plan was to add a helper (`git add--helper`) that takes
  over more and more responsibility from the Perl script over the course
  of the conversion.

  This plan is no longer in effect, as I encountered a serious problem
  with that: the MSYS2 runtime used by the Perl interpreter which Git
  for Windows employs to run `git add -i` has a curious bug (that is
  safely outside the purview of this here patch series) where it fails
  to read from standard input after it spawned a non-MSYS2 program that
  reads from standard input. To keep my `git add -i` in a working state,
  I therefore adopted a different strategy:

  Just like `git difftool` was converted by starting with a built-in
  that did nothing but handing off to the scripted version, guarded by
  the (opt-in) `difftool.useBuiltin` config setting, I start this patch
  series by a built-in `add -i` that does nothing else but state that it
  is not implemented yet, guarded by the (opt-in)
  `add.interactive.useBuiltin` config setting.

  In contrast to the `git difftool` situation, it is quite a bit easier
  here, as we do not even have to rename the script to
  `git-legacy-add--interactive.perl`: the `add--interactive` command is
  an implementation detail that users are not even supposed to know
  about. Therefore, we can implement that road fork between the built-in
  and the scripted version in `builtin/add.c`, i.e. in the user-facing
  `git add` command.

  This will also naturally help with the transition to a fully built-in
  `git add -i`/`git add -p`, as we saw with the built-in `git rebase`
  how important it is for end users to have an escape hatch (and for
  that reason, tried our best to provide the same with the built-in `git
  stash`).

- The `help` command was actually not hooked up in `git add -i`, but was
  only available as a special option of the `git add--helper` command.
  As that command no longer exists, I kind of *had* to implement
  some way to let the built-in `git add -i` show the help text.

- The main loop of `git add -i` (i.e. the thing that lets you choose
  `status` or `help`) is now implemented (but only lists `status` and
  `help`, of course), as it makes use of that feature that took the main
  chunk of the Outreachy project: the function to determine unique
  prefixes of a list of strings.

- Speaking of the unique prefixes: the functionality to determine those
  is now encapsulated in the `prefix-map.c` file, and I also added a
  regression test.

- Speaking of the tests: I also implemented support for the environment
  variable `GIT_TEST_ADD_I_USE_BUILTIN`: by setting it, the test suite
  can be forced to use the built-in, or the Perl script, version of `git
  add -i`. Needless to say: by the end of this patch series, running the
  test suite with `GIT_TEST_ADD_I_USE_BUILTIN=true` will still result in
  a ton of test failures due to not-yet-implemented commands, but it
  will also demonstrate what *already* works.

- Since the main loop starts not only by showing the status, but
  refreshes the index before that, I added that, and I actually
  refactored that code into a new function
  (`repo_refresh_and_write_index()`), as it will be used a couple of
  times by the end of the complete conversion of `git add -i` into a
  built-in command.

Signed-off-by: Johannes Schindelin <[email protected]>
This patch series implements the rest of the commands in `git add -i`'s
main loop: `update`, `revert`, `add_untracked`, `patch`, `diff`, and
`quit`. Apart from `quit`, these commands are all very similar in that
they first build a list of files, display it, let the user choose which
ones to act on, and then perform the action.

Note that the `patch` command is not actually converted to C, not
completely at least: the built-in version simply hands off to `git
add--interactive` after letting the user select which files to act on.

The reason for this omission is practicality. Out of the 1,800+ lines of
`git-add--interactive.perl`, over a thousand deal *just* with the `git
add -p` part. I did convert that functionality already (to be
contributed in a separate patch series), discovering that there is so
little overlap between the `git add --patch` part and the rest of `git
add --interactive` that I could put the former into a totally different
file: `add-patch.c`. Just a teaser ;-)

Signed-off-by: Johannes Schindelin <[email protected]>
While re-implementing `git add -i` and `git add -p` in C, I tried to
make sure that there is test coverage for all of the features I convert
from Perl to C, to give me some confidence in the correctness from
running the test suite both with `GIT_TEST_ADD_I_USE_BUILTIN=true` and
with `GIT_TEST_ADD_I_USE_BUILTIN=false`.

However, I discovered that there are a couple of gaps. This patch series
intends to close them.

The first patch might actually not be considered a gap by some: it
basically removes the need for the `TTY` prerequisite in the `git add
-i` tests to verify that the output is colored all right. This change is
rather crucial for me, though: on Windows, where the conversion to a
built-in shows the most obvious benefits, there are no pseudo terminals
(yet), therefore `git.exe` cannot work with them (even if the MSYS2 Perl
interpreter used by Git for Windows knows about some sort of pty
emulation). And I *really* wanted to make sure that the colors work on
Windows, as I personally get a lot out of those color cues.

The patch series ends by addressing two issues that are not exactly
covering testing gaps:

- While adding a test case, I noticed that `git add -p` exited with
  *success* when it could not even generate a diff. This is so obviously
  wrong that I had to fix it right away (I noticed, actually, because my
  in-progress built-in `git add -p` failed, and the Perl version did
  not), and I used the same test case to verify that this is fixed once
  and for all.

- While working on covering those test gaps, I noticed a problem in an
  early version of the built-in version of `git add -p` where the `git
  apply --allow-overlap` mode failed to work properly, for little
  reason, and I fixed it real quick.

  It would seem that the `--allow-overlap` function is not only
  purposefully under-documented, but also purposefully under-tested,
  probably to discourage its use. I do not quite understand what I
  perceive to be Junio's aversion to that option, but I did not feel
  like I should put up a battle here, so I did not accompany this fix
  with a new test script.

  In the end, the built-in version of `git add -p` does not use the
  `--allow-overlap` function at all, anyway. Which should make everybody
  a lot happier.

Signed-off-by: Johannes Schindelin <[email protected]>
Out of all the patch series on the journey to provide `git add
--interactive` and `git add --patch` in a built-in versions, this is the
big one, as can be expected from the fact that the `git add --patch`
functionality makes up over half of the 1,800+ lines of
`git-add--interactive.perl`.

The two patches that stick out are of course the ones to implement hunk
splitting and hunk editing: these operations are fundamentally more
complicated, and less obvious, than the rest of the operations.

Signed-off-by: Johannes Schindelin <[email protected]>
At this stage on the journey to a fully built-in `git add`, we already
have everything we need, including the `--interactive` and `--patch`
options, as long as the `add.interactive.useBuiltin` setting is set to
`true` (kind of a "turned off feature flag", which it will be for a
while, until we get confident enough that the built-in version does the
job, and retire the Perl script).

However, the internal `add--interactive` helper is also used to back the
`--patch` option of `git stash`, `git reset` and `git checkout`.

This patch series brings them "online".

Signed-off-by: Johannes Schindelin <[email protected]>
This is the final leg of the journey to a fully built-in `git add`: the
`git add -i` and `git add -p` modes were re-implemented in C, but they
lacked support for a couple of config settings.

The one that sticks out most is the `interactive.singleKey` setting: it
was not only particularly hard to get to work, especially on Windows. It
is also the setting that seems to be incomplete already in the Perl
version: while the name suggests that it applies to the main loop of
`git add --interactive`, or to the file selections in that command, it
does not. Only the `git add --patch` mode respects that setting.

As it is outside the purpose of the conversion of
`git-add--interactive.perl` to C, we will leave that loose end for some
future date.

Signed-off-by: Johannes Schindelin <[email protected]>
This topic branch fixes a corner case that is amazingly common in this
developer's workflow: in a `git stash -p`, splitting a hunk and stashing
only part of it runs into a (known) bug where the partial hunk cannot be
applied in reverse.

It is one of those "good enough" fixes, not a full fix, though, as the
full fix would require a 3-way merge between `stash^` and the *worktree*
(not `HEAD`), with `stash` as merge base (i.e. a `git revert`, but on
top of the current worktree).

Signed-off-by: Johannes Schindelin <[email protected]>
This function is marked as `NORETURN`, and it indeed does not want to
return anything. So let's not declare it with the return type `int`.
This fixes the following warning when building with MSVC:

	C4646: function declared with 'noreturn' has non-void return type

Signed-off-by: Johannes Schindelin <[email protected]>
MSVC complains about this with `-Wall`, which can be taken as a sign
that this is indeed a real bug. The symptom is:

	C4146: unary minus operator applied to unsigned type, result
	still unsigned

Let's avoid this warning in the minimal way, e.g. writing `-1 -
<unsigned value>` instead of `-<unsigned value> - 1`.

Note that the change in the `estimate_cache_size()` function is
needed because MSVC considers the "return type" of the `sizeof()`
operator to be `size_t`, i.e. unsigned, and therefore it cannot be
negated using the unary minus operator.

Even worse, that arithmetic is doing extra work, in vain. We want to
calculate the entry extra cache size as the difference between the
size of the `cache_entry` structure minus the size of the
`ondisk_cache_entry` structure, padded to the appropriate alignment
boundary.

To that end, we start by assigning that difference to the `per_entry`
variable, and then abuse the `len` parameter of the
`align_padding_size()` macro to take the negative size of the ondisk
entry size. Essentially, we try to avoid passing the already calculated
difference to that macro by passing the operands of that difference
instead, when the macro expects operands of an addition:

	#define align_padding_size(size, len) \
		((size + (len) + 8) & ~7) - (size + len)

Currently, we pass A and -B to that macro instead of passing A - B and
0, where A - B is already stored in the `per_entry` variable, ready to
be used.

This is neither necessary, nor intuitive. Let's fix this, and have code
that is both easier to read and that also does not trigger MSVC's
warning.

Signed-off-by: Johannes Schindelin <[email protected]>
MSVC would complain thusly:

    C4200: nonstandard extension used: zero-sized array in struct/union

Let's just use the `FLEX_ARRAY` constant that we introduced for exactly
this type of scenario.

Signed-off-by: Johannes Schindelin <[email protected]>
This adds the common guards that allow headers to be #include'd multiple
times.

Signed-off-by: Johannes Schindelin <[email protected]>
To build with MSVC, we "translate" GCC options to MSVC options, and part
of those options refer to the libraries to link into the final
executable. Currently, this part looks somewhat like this on Windows:

	-lcurl -lnghttp2 -lidn2 -lssl -lcrypto -lssl -lcrypto -lgdi32
	-lcrypt32 -lwldap32 -lz -lws2_32 -lexpat

Some of those are direct dependencies (such as curl and ssl) and others
are indirect (nghttp2 and idn2, for example, are dependencies of curl,
but need to be linked in for reasons).

We already handle the direct dependencies, e.g. `-liconv` is already
handled as adding `libiconv.lib` to the list of libraries to link
against.

Let's just ignore the remaining `-l*` options so that MSVC does not have
to warn us that it ignored e.g. the `/lnghttp2` option. We do that by
extending the clause that already "eats" the `-R*` options to also eat
the `-l*` options.

Signed-off-by: Johannes Schindelin <[email protected]>
We frequently build Git using the `DEVELOPER=1` make setting as a
shortcut to enable all kinds of more stringent compiler warnings.

Those compiler warnings are relatively specific to GCC, though, so let's
try our best to translate them to the equivalent options to pass to MS
Visual C++'s `cl.exe`.

Signed-off-by: Johannes Schindelin <[email protected]>
The return value of that function is 0 both for variables that are
unset, as well as for variables whose values are empty. To discern those
two cases, one has to call `GetLastError()`, whose return value is
`ERROR_ENVVAR_NOT_FOUND` and `ERROR_SUCCESS`, respectively.

Except that it is not actually set to `ERROR_SUCCESS` in the latter
case, apparently, but the last error value seems to be simply unchanged.

To work around this, let's just re-set the last error value just before
inspecting the environment variable.

This fixes a problem that triggers failures in t3301-notes.sh (where we
try to override config settings by passing empty values for certain
environment variables).

This problem is hidden in the MINGW build by the fact that older
MSVC runtimes (such as the one used by MINGW builds) have a `calloc()`
that re-sets the last error value in case of success, while newer
runtimes set the error value only if `NULL` is returned by that
function.

Signed-off-by: Johannes Schindelin <[email protected]>
This was a left-over from the previous YAML schema, and it no longer
works. The problem was noticed while editing `azure-pipelines.yml` in VS
Code with the very helpful "Azure Pipelines" extension (syntax
highlighting and intellisense for `azure-pipelines.yml`...).

Signed-off-by: Johannes Schindelin <[email protected]>
... because we can, now.

We specifically reduce the number of parallel links for MSVC, as RAM
usage is an issue with MSVC's parallel mode, manifested in the symptom:

	fatal error LNK1318: Unexpected PDB error; OK (0) ''

Signed-off-by: Johannes Schindelin <[email protected]>
From the documentation of said setting:

	This boolean will enable fsync() when writing object files.

	This is a total waste of time and effort on a filesystem that
	orders data writes properly, but can be useful for filesystems
	that do not use journalling (traditional UNIX filesystems) or
	that only journal metadata and not file contents (OS X’s HFS+,
	or Linux ext3 with "data=writeback").

The most common file system on Windows (NTFS) does not guarantee that
order, therefore a sudden loss of power (or any other event causing an
unclean shutdown) would cause corrupt files (i.e. files filled with
NULs). Therefore we need to change the default.

Note that the documentation makes it sound as if this causes really bad
performance. In reality, writing loose objects is something that is done
only rarely, and only a handful of files at a time.

Signed-off-by: Johannes Schindelin <[email protected]>
To avoid running into command line limitations, some of Git's commands
support the `--stdin` option.

Let's use exactly this option in the three rev-list/log invocations in
gitk that would otherwise possibly run the danger of trying to invoke a
too-long command line.

While it is easy to redirect either stdin or stdout in Tcl/Tk scripts,
what we need here is both. We need to capture the output, yet we also
need to pipe in the revs/files arguments via stdin (because stdin does
not have any limit, unlike the command line). To help this, we use the
neat Tcl feature where you can capture stdout and at the same time feed
a fixed string as stdin to the spawned process.

One non-obvious aspect about this change is that the `--stdin` option
allows to specify revs, the double-dash, and files, but *no* other
options such as `--not`. This is addressed by prefixing the "negative"
revs with `^` explicitly rather than relying on the `--not` option
(thanks for coming up with that idea, Max!).

This fixes #1987

Analysis-and-initial-patch-by: Max Kirillov <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
When grepping through the output of a command in the test suite, there
is always a chance that something goes wrong, in which case there would
not be anything useful to debug.

Let's redirect the output into a file instead, and grep that file, so
that the log can be inspected easily if the grep fails.

Signed-off-by: Johannes Schindelin <[email protected]>
For performance reasons `stdout` is buffered by default. That leads to
problems if after printing to `stdout` a read on `stdin` is performed.

For that reason interactive commands like `git clean -i` do not function
properly anymore if the `stdout` is not flushed by `fflush(stdout)` before
trying to read from `stdin`.

So let's precede all reads on `stdin` in `git clean -i` by flushing
`stdout`.

Signed-off-by: 마누엘 <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
dscho and others added 28 commits August 17, 2019 00:15
Handle Ctrl+C in Git Bash nicely

Signed-off-by: Johannes Schindelin <[email protected]>
This branch allows third-party tools to call `git status
--no-lock-index` to avoid lock contention with the interactive Git usage
of the actual human user.

Signed-off-by: Johannes Schindelin <[email protected]>
…gracefully

Phase out `--show-ignored-directory` gracefully
Add a README.md for GitHub goodness.

Signed-off-by: Johannes Schindelin <[email protected]>
This is the recommended way on GitHub to describe policies revolving around
security issues and about supported versions.

Signed-off-by: Johannes Schindelin <[email protected]>
SECURITY.md: document Git for Windows' policies
no longer relevant after moving to PCRE2

Signed-off-by: Carlo Marcelo Arenas Belón <[email protected]>
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.

This poses a problem in particular on NTFS/FAT, where e.g. the colon
character is not a valid 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 <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
rebase-merges: improve --rebase-merges label generation
Let's refactor the `validate_system_file_ownership()` a bit, not only to
clarify what is done, but also to prepare for maybe adding a slighty
more expensive check: it is possible that the file
`C:\ProgramData\Git\config` is owned by the local administrator (who has
a different SID than the `Administrators` group).

Signed-off-by: Johannes Schindelin <[email protected]>
We recently hardened Git for Windows by ignoring
`C:\ProgramData\Git\config` files unless they are owned by the system
account, by the administrators group, or by the account under which Git
is running.

Turns out that there are situations when that config file is owned by
_an_ administrator, not by the entire administrators group, and that
still is okay. This can happen very easily e.g. in Docker Containers.

Let's add a fall back when the owner is none of the three currently
expected ones, enumerating the members of the administrators group and
comparing them to the file's owner. If a match is found, the owner is
not dubious.

Enumerating groups' members on Windows is not exactly a cheap operation,
and it requires linking to the `netapi32.dll`. Since this is rarely
needed, and since this is done at most a handful of times during any Git
process' life cycle, it is okay that it is a bit expensive. To avoid the
startup cost of linking to yet another DLL, we do this lazily instead:
that way, the vast majority of Git for Windows' users will not feel any
impact by this patch.

This fixes #2304.

Signed-off-by: Johannes Schindelin <[email protected]>
gitk: Escape file paths before piping to git log
As suggested in
#2303 (comment):

Also mention the release candidate and snapshot version numberings, e.g.
that the final release's installer will claim that the release candidates
are newer than the proper release.

And also note the existence of the snapshots; This may encourage others
to participate in the 'development'.

Signed-off-by: Philip Oakley <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Fix a few more unclear/incorrect phrasings (while the perfect is the
enemy of the good, the vague and the not-quite-right are the enemy of
the good-enough).

Signed-off-by: Johannes Schindelin <[email protected]>
Security.md: include release candidate and snapshot information
…ific-administrator

Do not complain if `C:\ProgramData\Git\config` is owned by a specific administrator
The vcpkg_install batch file depends on the availability of a
working Git on the CMD path. This may not be present if the user
has selected the 'bash only' option during Git-for-Windows install.

Detect and tell the user about their lack of a working Git in the CMD
window.

Fixes #2348.
A separate PR git-for-windows/build-extra#258
now highlights the recommended path setting during install.

Signed-off-by: Philip Oakley <[email protected]>
The vcpkg downloads may not succeed. Warn careful readers of the time out.

A simple retry will usually resolve the issue.

Signed-off-by: Philip Oakley <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Vcpkg Install: detect lack of working Git, and note possible vcpkg time outs
This moves the system config into a more logical location: the `mingw64`
part of `C:\Program Files\Git\mingw64\etc\gitconfig` never made sense,
as it is a mere implementation detail. Let's skip the `mingw64` part and
move this to `C:\Program Files\Git\etc\gitconfig`.

Side note: in the rare (and not recommended) case a user chooses to
install 32-bit Git for Windows on a 64-bit system, the path will of
course be `C:\Program Files (x86)\Git\etc\gitconfig`.

The next step after this patch is to scrap support for
`C:\ProgramData\Git\config`, which never really made sense to users, and
which is inconsistent with non-Windows versions of Git, anyway.

Background: During the Git for Windows v1.x days, the system config was
located at `C:\Program Files (x86)\Git\etc\gitconfig`. With Git for
Windows v2.x, it moved to `C:\Program Files\Git\mingw64\gitconfig` (or
`C:\Program Files (x86)\Git\mingw32\gitconfig`).

Obviously, this new location was not stable (because of the "mingw64"
part) and this maintainer thought that it was a splendid time to
introduce a "super-system" config, with a constant location, that would
be shared by all Git implementations. Hence `C:\ProgramData\Git\config`
was born.

What we *should* have done instead is to move the location to the
top-level `etc` directory instead of the `mingw64\etc` one (or
`mingw32\etc` for 32-bit versions).

Likewise, we should have treated the system `gitattributes` in a similar
manner.

This patch makes it so.

Obviously, we are cautious to do this only for the known install
locations `/mingw64` and `/mingw32`; If anybody wants to override that
while building their version of Git (e.g. via `make prefix=$HOME`), we
leave the default location of the system config and gitattributes alone.

Signed-off-by: Johannes Schindelin <[email protected]>
The CI builds are failing for Mac OS X due to a change in the
location of the perforce cask. The command outputs the following
error:

    + brew install caskroom/cask/perforce
    Error: caskroom/cask was moved. Tap homebrew/cask-cask instead.

Preface the "brew install caskroom/cask/perforce" with the old
way of installing perforce, and only try this method if the
"brew install perforce" fails.

The existing way to use caskroom was added in 672f51c (travis-ci:
fix Perforce install on macOS, 2017-01-22) and the justification
is that the "brew install perforce" can fail due to a hash
mis-match. The mismatch is due to the official Perforce distro
updating the published binaries without updating the version
string. CI servers are typically fresh virtual machines, so that
issue should not arise in automated builds.

Even if a build server is re-used and hits the hash mis-match,
it will fall back to the "new" mechanism which is currently
failing, but may be fixed independently of this change.

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Derrick Stolee <[email protected]>
In the previous commit, we moved the location of the system config to
`C:\Program Files\Git\etc\gitconfig`, to give users a reliable location
for the system config.

Previously, we had introduced `C:\ProgramData\Git\config` as such a
location.

However, the purpose of `C:\ProgramData\Git\config` was always for the
installed Git for Windows to share its configured options. Every upgrade
will modify the settings in that file according to what the user
specified when running Git for Windows' installer. So it is not really a
file whose ownership is shared between all Git implementations, contrary
to what the location suggests.

Also, it was totally confusing to many users that there was a "system"
config and then yet another "sort of system" config that did not even
have a corresponding `git config` option such as `--system`.

Further, Portable Git should probably never look at
`C:\ProgramData\Git\config` to begin with: this makes it rather
non-Portable ;-)

Finally, support for `C:\ProgramData\Git\config` never really caught on:
only libgit2 implemented it, but even JGit (which might be used by more
users than libgit2-based applications by grace of backing Android
Studio) did not.

Therefore, let's finally admit that it was a mistake to introduce
`C:\ProgramData\Git\config` and remove support for it.

This reverts commit bcbbb4f.

Signed-off-by: Johannes Schindelin <[email protected]>
It still fails! *Still!*

Even after upgrading the cask's definition in the PR
Homebrew/homebrew-cask#70981 does it fail.

The explanation is, as Gábor Szeder dug out from the commit history:
the local definitions might get stale and need to be updated.

So let's just update them in case `brew cask install perforce` fails,
and just try again!

This will still fail, of course, when `homebrew-cask` falls behind
Perforce's release schedule. But once it is updated, we can now simply
re-run the failed jobs and they will pick up that update.

As to updating `homebrew-cask`: I started automating this, via
https://dev.azure.com/gitgitgadget/git/_build?definitionId=11&_a=summary
(I plan on finishing it once the next Perforce upgrade comes around.)

Signed-off-by: Johannes Schindelin <[email protected]>
Work around that pesky Homebrew `perforce` problem
…amdata-config

Make `git config --system` work like you think it should on Windows
Auto-generated by `make vcxproj`
garimasi514 pushed a commit that referenced this pull request Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.