Aborting a git commit in a repo with pre-commit hooks from the git tab does not yield the same result as aborting from the command line

[lorenzwalthert]

System details

RStudio Edition : Desktop
RStudio Version : 1.2.5033
OS Version      :  macOS 10.15.3
R Version       :  3.6.1

Steps to reproduce the problem

• clone example repo with minimal example: git clone git@github.com:lorenzwalthert/rstudio-git-bug.git
• Install the pre-commit framework with
  remotes::install_github('lorenzwalthert/precommit')
• initiate the hooks with precommit::use_precommit().
• edit the two R files file1 and file2 and stage one.
• attempt to commit via the RStudio git tab, then abort immediately.
• The file that was not staged was stashed away during the pre-commit checks (by pre-commit) and should be restored afterwards according to pre-commit. This does not happen when committing via the RStudio git tab, but it happens when committing via the console.

Describe the problem in detail

https://pre-commit.com is an increasingly popular framework for managing git hooks, there are specific R hooks too (disclosure: I am the maintainer). Upon committing, it stashes all non-staged changes to a cache. Aborting to commit with the RStudio git tab makes all changes that are not staged for commit lost (unless one is a git expert). This is not the case when aborting from the command line because pre-commit restores the stashes in this case from the cache.

Suspicion: Aborting the commit via the RStudio git tab is implemented with KILL and not with INT or similar (pre-commit/pre-commit#1362 (comment)) so pre-commit cannot do the cleanup.

This behaviour makes it very dangerous to use pre-commit with RStudio and for that reason I think it should be fixed.

Reference: pre-commit/pre-commit#1362

Describe the behavior you expected

I'd expect the stash to be restored after aborting the commit, i.e. that aborting a git commit on the command line results in the same git status as aborting from the RStudio git tab.

• I have read the guide to submitting good bug reports at https://github.com/rstudio/rstudio/wiki/Writing-Good-Bug-Reports .
• I have installed the latest version of RStudio and confirmed that the issue still persists.
• If I am reporting a RStudio crash, I have included a diagnostics report. https://support.rstudio.com/hc/en-us/articles/200321257-Running-a-Diagnostics-Report
• I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.

cc: @asottile

[ronblum]

@lorenzwalthert Thanks! I can reproduce this with the code/steps above. I'll bump this up to triage for review as we continue development of RStudio.

[lorenzwalthert]

Thanks for the heads up @ronblum. I believe this is possibly a quick fix.

[pat-s]

@ronblum Is there any rough ETA for this fix?

[kevinushey]

@lorenzwalthert, it looks like your intuition here is basically correct.

Just to log some investigation here -- we have a ConsoleProcess class that handles these sub-processes, launched e.g. here:

rstudio/src/cpp/session/modules/SessionGit.cpp

Lines 430 to 460 in 1889a1b

	core::Error createConsoleProc(const ShellArgs& args,
	const std::string& caption,
	boost::shared_ptr<ConsoleProcess>* ppCP,
	const boost::optional<FilePath>& workingDir=boost::optional<FilePath>())
	{
	using namespace session::console_process;
	core::system::ProcessOptions options = procOptions();
	#ifdef _WIN32
	options.detachProcess = true;
	#endif
	if (!workingDir)
	options.workingDir = root_;
	else if (!workingDir.get().isEmpty())
	options.workingDir = workingDir.get();
	boost::shared_ptr<ConsoleProcessInfo> pCPI =
	boost::make_shared<ConsoleProcessInfo>(caption,
	console_process::InteractionNever);
	#ifdef _WIN32
	*ppCP = ConsoleProcess::create(gitBin(), args.args(), options, pCPI);
	#else
	*ppCP = ConsoleProcess::create(git() << args.args(), options, pCPI);
	#endif
	(*ppCP)->enquePrompt(gitText(args));
	(*ppCP)->onExit().connect(boost::bind(&enqueueRefreshEvent));
	return Success();
	}

When a process is cancelled, we set an interrupt flag here:

rstudio/src/cpp/session/SessionConsoleProcess.cpp

Lines 404 to 407 in 1889a1b

	void ConsoleProcess::interrupt()
	{
	interrupt_ = true;
	}

Which is eventually checked and read within our onContinue() handler:

rstudio/src/cpp/session/SessionConsoleProcess.cpp

Lines 422 to 424 in 1889a1b

	// full stop interrupt if requested
	if (interrupt_)
	return false;

But having that return false actually implies terminating (as opposed to interrupting) the process:

rstudio/src/cpp/core/system/PosixChildProcess.cpp

Lines 1029 to 1039 in 1889a1b

	// call onContinue
	if (callbacks_.onContinue)
	{
	if (!callbacks_.onContinue(*this))
	{
	// terminate the proces
	Error error = terminate();
	if (error)
	LOG_ERROR(error);
	}
	}

It seems like this could be fixed by sending a real interrupt to the process, although we'd of course need to check whether the current behavior is intentional for certain contexts.