The code in getHydratedRebasingCommits relied on the assumption that the
git-rebase-todo file contains full SHAs. This has only been true from 2.25.2 on,
before that it would contain abbreviated SHAs. Fix this by storing fullCommits
in a slice instead of a map, and using a linear search.
I don't know why we're getting index.lock errors but they're impossile to stop
anyway given that other processes can be calling git commands. So we're retrying
a few times before re-raising. To do this we need to clone the command and the current
implementation for that is best-effort.
I do worry about the maintainability of that but we'll see how it goes.
Also, I thought you'd need to clone the task (if it exists) but now I think not;
as long as you don't call done twice on it you should be fine, and you shouldn't
be done'ing a task as part of running a command: that should happen higher up.
It's not clear what was happening but it seemed like we sometimes weren't
fully writing to our stdout buffer (which is used for the error message)
even though we had returned from cmd.Wait().
Not sure what the cause was but removing an unnecessary goroutine fixed it.
The global counter approach is easy to understand but it's brittle and depends on implicit behaviour that is not very discoverable.
With a global counter, if any goroutine accidentally decrements the counter twice, we'll think lazygit is idle when it's actually busy.
Likewise if a goroutine accidentally increments the counter twice we'll think lazygit is busy when it's actually idle.
With the new approach we have a map of tasks where each task can either be busy or not. We create a new task and add it to the map
when we spawn a worker goroutine (among other things) and we remove it once the task is done.
The task can also be paused and continued for situations where we switch back and forth between running a program and asking for user
input.
In order for this to work with `git push` (and other commands that require credentials) we need to obtain the task from gocui when
we create the worker goroutine, and then pass it along to the commands package to pause/continue the task as required. This is
MUCH more discoverable than the old approach which just decremented and incremented the global counter from within the commands package,
but it's at the cost of expanding some function signatures (arguably a good thing).
Likewise, whenever you want to call WithWaitingStatus or WithLoaderPanel the callback will now have access to the task for pausing/
continuing. We only need to actually make use of this functionality in a couple of places so it's a high price to pay, but I don't
know if I want to introduce a WithWaitingStatusTask and WithLoaderPanelTask function (open to suggestions).
I don't know if this is a hack or not: we run a git command and increment the pending action
count to 1 but at some point the command requests a username or password, so we need to prompt
the user to enter that. At that point we don't want to say that there is a pending action,
so we decrement the action count before prompting the user and then re-increment it again afterward.
Given that we panic when the counter goes below zero, it's important that it's not zero
when we run the git command (should be impossible anyway).
I toyed with a different approach using channels and a long-running goroutine that
handles all commands that request credentials but it feels over-engineered compared to this
commit's approach.
The first line of the diff pane would show branch heads (e.g.
commit dd9100ccc8b69a8b14b21a84e34854b5acfb871a (mybranch, origin/mybranch)
only when a pager is used. The reason is that the default of the --decorate
option to git show is "auto", which means to show the decoration only when
output goes to a tty. Lazygit uses a pty only when a pager is used, so the
decoration wouldn't show when no pager is used.
Since the branch head annotation is useful and we always want to see it, force
it by explicitly passing --decorate.
This solves three problems:
1. When the local main branch is behind its upstream, the merged status of
commits of a feature branch sitting on origin/main was not correct. This can
easily happen when you rebase a branch onto origin/main instead of main, and
don't bother keeping local main up to date.
2. It works when you don't have the main branch locally at all. This could
happen when you check out a colleague's feature branch that goes off of
"develop", but you don't have "develop" locally yourself because you normally
only work on "main".
3. It also works when you work on a main branch itself, e.g. by committing to it
directly, or by merging a branch locally. These local commits on a main
branch would previously be shown in green instead of red; this broke with
910a61dc46.
When stopping in a rebase because of a conflict, it is nice to see the commit
that git is trying to apply. Create a fake todo entry labelled "conflict" for
this, and show the "<-- YOU ARE HERE ---" string for that one (in red) instead
of for the real current head.
Missed a spot a couple PR's ago. We had an integration test which caught this but which was skipped due
to index.lock file issues. The test was also broken for other reasons due to it not having been running
for a while, so I've fixed that up too.
By constructing an arg vector manually, we no longer need to quote arguments
Mandate that args must be passed when building a command
Now you need to provide an args array when building a command.
There are a handful of places where we need to deal with a string,
such as with user-defined custom commands, and for those we now require
that at the callsite they use str.ToArgv to do that. I don't want
to provide a method out of the box for it because I want to discourage its
use.
For some reason we were invoking a command through a shell when amending a
commit, and I don't believe we needed to do that as there was nothing user-
supplied about the command. So I've switched to using a regular command out-
side the shell there
The only exception is when moving a custom patch for an entire commit to an
earlier commit; in this case the source commit becomes empty, but we want to
keep it, mainly for consistency with moving the patch to a later commit, which
behaves the same.
In all other cases where we rebase, it's confusing when empty commits are kept;
the most common example is rebasing a branch onto master, where master already
contains some of the commits of our branch. In this case we simply want to drop
these.
It defaults to {"master", "main"}, but can be set to whatever branch names
are used as base branches, e.g. {"master", "devel", "v1.0-hotfixes"}. It is
used for color-coding the shas in the commit list, i.e. to decide whether
commits are green or yellow.
Previously, when rebasing a branch onto a newer master, all commits from the
previous fork point up to its head were marked red (unpushed), including the
commits that are on master already. While this is technically correct from the
perspective of the current branch's upstream, it's not what most people expect,
intuitively; they want to see where the current branch starts, relative to
master. So all commits of master should be green, and then the commits of the
current branch in red.
When we use the one panel for the entire commit message, its tricky to have a keybinding both for adding a newline and submitting.
By having two panels: one for the summary line and one for the description, we allow for 'enter' to submit the message when done from the summary panel,
and 'enter' to add a newline when done from the description panel. Alt-enter, for those who can use that key combo, also works for submitting the message
from the description panel. For those who can't use that key combo, and don't want to remap the keybinding, they can hit tab to go back to the summary panel
and then 'enter' to submit the message.
We have some awkwardness in that both contexts (i.e. panels) need to appear and disappear in tandem and we don't have a great way of handling that concept,
so we just push both contexts one after the other, and likewise remove both contexts when we escape.
This fixes two problems with the "amend commit with staged changes" command:
1. Amending to a fixup commit didn't work (this would create a commmit with the
title "fixup! fixup! original title" and keep that at the top of the branch)
2. Unrelated fixup commits would be squashed too.
The added integration test verifies that both of these problems are fixed.
Instead of passing a bunch of different options in
PrepareInteractiveRebaseCommandOpts, where it was unclear how they interact if
several are set, have only a single field "instruction" which can be set to one
of various different instructions.
The functionality of replacing the entire todo file with our own is no longer
available; it is only possible to prepend todos to the existing file.
Also, instead of using different env vars for the various rebase operations that
we want to tell the daemon to do, use a single one that contains a json-encoded
struct with all available instructions. This makes the protocol much clearer,
and makes it easier to extend in the future.
At the moment it doesn't make a big difference, because the vast majority of
callers create a list of todos themselves to completely replace what git came up
with. We're changing this in the following commits though, and then it's helpful
to preserve merges.
It used to work on the assumption that rebasing commits in lazygit's model
correspond one-to-one to lines in the git-rebase-todo file, which isn't
necessarily true (e.g. when users use "git rebase --edit-todo" at the custom
command prompt and add a "break" between lines).
This is useful when working with stacked branches, because you can now move
"pick" entries across an update-ref command and you can tell exactly which
branch the commit will end up in.
It's also useful to spot situations where the --update-refs option didn't work
as desired. For example, if you duplicate a branch and want to rebase only one
of the branches but not the other (maybe for testing); if you have
rebase.updateRefs=true in your git config, then rebasing one branch will move
the other branch along. To solve this we'll have to introduce a way to delete
the update-ref entry (maybe by hitting backspace?); this is out of scope for
this PR, so for now users will have to type "git rebase --edit-todo" into the
custom command prompt to sort this out.
We will also have to prevent users from trying to turn update-ref commands into
other commands like "pick" or "drop"; we'll do this later in this branch.
So far the algorithm worked on the assumption that the output of the "git show"
command corresponds one-to-one to the lines of the rebase-todo file. This
assumption doesn't hold once we start to include todo lines that don't have a
sha (like update-ref), or when the todo file contains multiple entries for the
same sha. This should never happen normally, but it can if users manually edit
the todo file and duplicate a line.
The main reason for doing this (besides the reasons given for Status in the
previous commit) is that it allows us to easily convert from TodoCommand to
Action and back. This will be needed later in the branch. Fortunately,
TodoCommand is one-based, so this allows us to add an ActionNone constant with
the value 0.
This is unrelated to the changes in this PR, but since we are doing the same
thing for the commit.Action field in the next commit, it makes sense to do it
for Status too for consistency. Modelling this as an enum feels more natural
than modelling it as a string, since there's a finite set of possible values.
And it saves a little bit of memory (not very much, since none of the strings
were heap-allocated, but still).
Instead, query the platform defaults only if the config is empty. This will be
necessary later to distinguish an empty config from a default config, so that we
can give deprecation warnings.
The "open" command is supposed to behave in the same way as double-clicking a
file in the Finder/Explorer. The concept of jumping to a specific line in the
file doesn't make sense for this; use "edit" instead.
These files were renamed from os_windows_test.go to os_test_windows.go (etc.) in
95b2e9540a. Since then, the tests have no longer run, since go only looks for
tests in files ending with "test.go".
It isn't important that the file name ends with "_windows.go", since there are
already build constrains in the files themselves.
Pulled this out into a separate commit since it was unrelated to the
feature coming behind it.
This just cleans up the `commit_test.go` file slightly (for the method
that I was working on) so that the tests are built in a way that is
slightly more readable - testing each configuration option individually
without combining any of them.
This fixes the problem that patching would stop at the first file that has a
conflict. We always want to patch all files.
Also, it's faster for large patches, and the code is a little bit simpler too.
All callers in this file now use reverseOnGenerate=false and
keepOriginalHeader=true, so hard-code that in the call to ModifiedPatchForLines
and get rid of the parameters.
The loop is pointless for two reasons:
- git apply --3way has this fallback built in already. If it can't do a
three-way merge, it will fall back to applying the patch normally.
- However, the only situation where it does this is when it can't do a 3-way
merge at all because it can't find the necessary ancestor blob. This can only
happen if you transfer a patch between different repos that don't have the
same blobs available; we are applying the patch to the same repo that is was
just generated from, so a 3-way merge is always possible. (Now that we fixed
the bug in the previous commit, that is.)
But the retry loop is not only pointless, it was actually harmful, because when
a 3-way patch fails with a conflict, git will put conflict markers in the
patched file and then exit with a non-zero exit status. So the retry loop would
try to patch the already patched file again, and this almost certainly fails,
but with a cryptic error message such as "error: main.go: does not exist in
index".
There's no reason to have two different ways of applying patches for whole-file
patches and partial patches; use --reverse for both. Not only does this simplify
the code a bit, but it fixes an actual problem: when reverseOnGenerate and
keepOriginalHeader are both true, the generated patch header is broken (the two
blobs in the line `index 6d1959b..6dc5f84 100644` are swapped). Git fails to do
a proper three-way merge in that case, as it expects the first of the two blobs
to be the common ancestor.
It would be possible to fix this by extending ModifiedPatchForLines to swap the
two blobs in this case; but this would prevent us from concatenating all patches
and apply them in one go, which we are going to do later in the branch.
We are going to add one more flag in the next commit.
Note that we are not using the struct inside patch_manager.go; we keep passing
the individual flags there. The reason for this will become more obvious later
in this branch.
Instead, derive it from context at display time (if we're rebasing, it's the
first non-todo commit). This fixes the problem that unfolding the current
commit's files in the local commits panel would show junk in the frame's title.
Along the way we make sure to only display the "<--- YOU ARE HERE" string in the
local commits panel; previously it would show for the top commit of a branch or
tag if mid-rebase.
It's not so much the total number of commits that matters here, it's just
whether we are on the first one. (This includes the other condition.)
This allows us to get rid of the condition in rebase.go.
Instead of rebasing from the commit below the current one and then setting the
current one to "edit", we rebase from the current one and insert a "break" after
it. In most cases the behavior is exactly the same as before, except that the
new method also works if the current commit is a merge commit. This is useful if
you want to create a new commit at the very beginning of your branch (by editing
the last commit before your branch).
I noticed that `$GIT_SEQUENCE_EDITOR` is overridden in `PrepareInteractiveRebaseCommand`
but not in `runSkipEditorCommand`.
Before this change, some commands such as `SquashAllAboveFixupCommits`
would not work when a different sequence editor, e.g.
[git-interactive-rebase-tool](https://github.com/MitMaro/git-interactive-rebase-tool)
is configured.
The 8.2 release of OpenSSH added support for FIDO/U2F hardware
authenticators, which manifests in being able to create new types of SSH
key, named `ecdsa-sk` nad `ed25519-sk`. This is relevant to lazygit,
as those SSH keys can be used to authorise git operations over SSH, as
well as signing git commits. Actual code changes are required for
correct support, as the authentication process for these types of keys
is different than the process for types supported previously.
When an operation requiring credentials is initialised with a U2F
authenticator-backed key, the first prompt is:
Enter PIN for ${key_type} key ${path_to_key}:
at which point the user is supposed to enter a numeric (and secret) PIN,
specific to the particular FIDO/U2F authenticator using which the SSH
keypair was generated. Upon entering the correct key, the user is
supposed to physically interact with the authenticator to confirm
presence. Sometimes this is accompanied by the following text prompt:
Confirm user presence for key ${key_type} ${key_fingerprint}
This second prompt does not always occur and it is presumed that the
user will know to perform this step even if not prompted specifically.
At this stage some authenticator devices may also begin to blink a LED
to indicate that they're waiting for input.
To facilitate lazygit's interoperability with these types of keys, add
support for the first PIN prompt, which allows "fetch", "pull", and
"push" git operations to complete.
If the remote name contains special regex-chars,
the compilation of the regex might fail.
Quoting the remoteName ensures that all special chars
in the remoteName are properly escaped before compiling
the regex.
test: add an integration test for checkout branch by name
fix: fix full ref name of detached head
refactor: refactor current branch loader
chore: use field name explicitly
