Re: [PATCH 2/2] builtin/history: implement "drop" subcommand

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> A common operation when editing the commit history is to drop a specific
> commit from the history entirely, but this operation is not currently
> covered by git-history(1).
>
> A couple of noteworthy bits:
>
>   - This is the first git-history(1) command that will ultimately result
>     in changes to both the index and the working tree. We thus have to
>     add logic to merge resulting changes into those.
>
>   - It is still not possible to replay merge commits, so this limitation
>     is inherited for the new "drop" command.
>
>   - For now we refuse to drop root commits. While we _can_ indeed drop
>     root commits in the general case, there are edge cases where the
>     resulting history would become completely empty. This is thus left
>     to a subsequent patch series.
>
> Other than that, most of the logic is rather straight-forward as we can
> continue to build on the preexisting logic in git-history(1) for most of
> the part.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ...
> +static int update_worktree(struct repository *repo,
> +			   const struct commit *old_head,
> +			   const struct commit *new_head,
> +			   bool dry_run)
> +{
> +...
> +
> +out:
> +	clear_unpack_trees_porcelain(&opts);
> +	rollback_lock_file(&lock);
> +	release_index(&index);
> +	free(desc_buf[0]);
> +	free(desc_buf[1]);
> +	return ret;
> +}

The function looks very familiar---anybody who wants to perform
"checkout <other-commit>" needs to do exactly the above.  It is a
bit surprising and disappointing that this topic needs to *invent*
its own helper function and carry it as a file-scope static.

> +	if (head_moves && update_worktree(repo, old_head, new_head, false) < 0) {
> +		ret = error(_("failed to update working tree; "
> +			      "run `git checkout HEAD` to sync"));
> +		goto out;
> +	}

This is minor, but unlike in documentation pages written in AsciiDoc, we do
not do backticks for literals in our error messages, I think.