git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Phillip Wood <phillip.wood123@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Elijah Newren <newren@gmail.com>, Duy Nguyen <pclouds@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH 1/2] commit/reset: try to clean up sequencer state
Date: Mon, 01 Apr 2019 17:28:43 +0900
Message-ID: <xmqq36n2yvr8.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190329163009.493-2-phillip.wood123@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When cherry-picking or reverting a sequence of commits and if the final
> pick/revert has conflicts and the user uses `git commit` to commit the
> conflict resolution and does not run `git cherry-pick --continue` then
> the sequencer state is left behind. This can cause problems later. In my
> case I cherry-picked a sequence of commits the last one of which I
> committed with `git commit` after resolving some conflicts, then a while
> later, on a different branch I aborted a revert which rewound my HEAD to
> the end of the cherry-pick sequence on the previous branch.

I've certainly seen this myself.  Do you use command line prompt
support to remind you of the operation in progress?  I do, and I
have a suspicion that it did not help me in this situation by
ceasing to tell me that I have leftover state files after a manual
commit of the final step that conflicted and gave control back to
me.

And detecting that we are finishing the last step and making sure
that the state files are removed does sound like the right approach
to fix it.

> diff --git a/branch.c b/branch.c
> index 28b81a7e02..9ed60081c1 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -5,6 +5,7 @@
>  #include "refs.h"
>  #include "refspec.h"
>  #include "remote.h"
> +#include "sequencer.h"
>  #include "commit.h"
>  #include "worktree.h"
>  
> @@ -339,8 +340,10 @@ void create_branch(struct repository *r,
>  
>  void remove_branch_state(struct repository *r)
>  {
> -	unlink(git_path_cherry_pick_head(r));
> -	unlink(git_path_revert_head(r));
> +	if (!unlink(git_path_cherry_pick_head(r)))
> +		sequencer_post_commit_cleanup();
> +	if (!unlink(git_path_revert_head(r)))
> +		sequencer_post_commit_cleanup();

This and the same one in builtin/commit.c feels a bit iffy.  If we
had CHERRY_PICK_HEAD or REVERT_HEAD and attempted to remove one or
the other, whether the removal succeeds or fails (perhaps a virus
scanner on Windows had the file open while we tried to unlink it,
causing the unlink() to fail), don't we want the clean-up to happen?

> @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	if (amend && !no_post_rewrite) {
>  		commit_post_rewrite(the_repository, current_head, &oid);
>  	}
> +

This is an unrelated change.

>  	if (!quiet) {
>  		unsigned int flags = 0;
>  
> diff --git a/sequencer.c b/sequencer.c
> index 0db410d590..028699209f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
>  	return len;
>  }
>  
> +void sequencer_post_commit_cleanup(void)
> +{
> +	struct replay_opts opts = REPLAY_OPTS_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *eol;
> +	const char *todo_path = git_path_todo_file();
> +
> +	if (strbuf_read_file(&buf, todo_path, 0) < 0) {
> +		if (errno == ENOENT) {
> +			return;
> +		} else {
> +			error_errno("unable to open '%s'", todo_path);
> +			return;
> +		}
> +	}
> +	/* If there is only one line then we are done */
> +	eol = strchr(buf.buf, '\n');
> +	if (!eol || !eol[1])
> +		sequencer_remove_state(&opts);
> +
> +	strbuf_release(&buf);
> +}

I find this helper doing a bit too much and a bit too little at the
same time.  To reduce the iffiness I mentioned earlier, the callers
would behefit to have a helper that

 - notices the need to remove CHERRY_PICK_HEAD or REVERT_HEAD, and
   returns without doing anything if there is no need;

 - remove the *_HEAD file.

 - detect if we have dealt with the last step, and returns without
   doing any more thing if there are more to do;

 - remove the state files.

IOW, replace the existing series of two unlink() calls with a single
call to the helper.

On the other hand, the bulk of hand-rolled logic to determine if we
have processed the last step is better done in another helper
function that helps this helper, i.e.

	void sequencer_post_commit_cleanup(struct repo *r)
	{
		int need_cleanup = 0;

		if (file_exists(git_path_cherry_pick_head(r)) {
			unlink(git_path_cherry_pick_head(r));
			need_cleanup = 1;
		}
		if (file_exists(git_path_revert_head(r)) {
			unlink(git_path_revert_head(r));
			need_cleanup = 1;
		}
		if (!need_cleanup)
			return;
		if (!have_finished_the_last_pick())
			return;
		sequencer_remove_state(&opts);
	}

as that makes it easier to follow the logic of what is going on at
this level, while at the same time making the logic in the
have_finished_the_last_pick() helper easier to read by giving it a
meaningful name.

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-29 16:30 [PATCH 0/2] A couple of cherry-pick related fixes Phillip Wood
2019-03-29 16:30 ` [PATCH 1/2] commit/reset: try to clean up sequencer state Phillip Wood
2019-04-01  8:28   ` Junio C Hamano [this message]
2019-04-08 14:15     ` Phillip Wood
2019-04-01 10:09   ` Duy Nguyen
2019-04-08 15:47     ` Phillip Wood
2019-03-29 16:30 ` [PATCH 2/2] fix cherry-pick/revert status after commit Phillip Wood
2019-04-01  8:34   ` Junio C Hamano
2019-04-08 14:17     ` Phillip Wood
2019-04-16 10:18 ` [PATCH v2 0/2] A couple of cherry-pick related fixes Phillip Wood
2019-04-16 10:18   ` [PATCH v2 1/2] commit/reset: try to clean up sequencer state Phillip Wood
2019-04-17  7:04     ` Junio C Hamano
2019-04-17 12:26       ` Johannes Schindelin
2019-04-17 15:04         ` Phillip Wood
2019-04-16 10:18   ` [PATCH v2 2/2] fix cherry-pick/revert status after commit Phillip Wood

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq36n2yvr8.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=phillip.wood123@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox