Ramkumar Ramachandra wrote:
> Since d3f4628e (revert: Remove sequencer state when no commits are
> pending, 2011-07-06), the sequencer removes the sequencer state before
> the final commit is actually completed. This design is inherently
> flawed, as it will not allow the user to abort the sequencer operation
> at that stage. Instead, make 'git commit' notify the sequencer after
> every successful commit; the sequencer then removes the state if no
> more instructions are pending.
Sorry, I'm getting lost in all the words. I suspect you are saying
“d3f4628e was trying to solve such-and-such problem, but its fix was
problematic because it removes the data that a hypothetical "git
cherry-pick --abort" command would need to work. Back out that
change and adopt the following instead.”
In particular, the above does not make it clear to me:
- as a user, what effect will I notice after this change?
- what problem does it solve?
- does it have any downsides?
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -26,6 +26,7 @@
> #include "unpack-trees.h"
> #include "quote.h"
> #include "submodule.h"
> +#include "sequencer.h"
>
> static const char * const builtin_commit_usage[] = {
> "git commit [options] [--] <filepattern>...",
> @@ -1521,6 +1522,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> unlink(git_path("MERGE_MODE"));
> unlink(git_path("SQUASH_MSG"));
>
> + /*
> + * Notify the sequencer that we're committing. The routine
> + * removes the sequencer state if our commit just completed
> + * the last instruction.
> + */
> + sequencer_notify_commit();
> +
> if (commit_index_files())
> die (_("Repository has been updated, but unable to write\n"
> "new_index file. Check that disk is not full or quota is\n"
The function name (..._notify_commit()) does not seem very intuitive.
Based on the name, I expect it to use the sequencer to print a message
to the user about the commit in progress.
What happens if writing to .git/index fails? I can think of reasons
to remove the sequencer file before or afterward:
- before, because once .git/index has been removed, the index is not
locked any more and further commands could take place in parallel.
- afterward, because when writing the index fails, I (the user)
might want to react by running "git cherry-pick --abort".
The latter seems slightly more compelling to me --- after all, any
further command wanting to touch the sequencer directory is going
to check whether it exists --- but more importantly, the former
reminds me that we haven't thought carefully about what concurrent
operations using the sequencer are and aren't allowed. Though I
doubt that it would come up much in practice. :)
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -580,6 +580,17 @@ static void read_populate_todo(struct replay_insn_list **todo_list)
> die(_("Unusable instruction sheet: %s"), todo_file);
> }
>
> +void sequencer_notify_commit(void)
> +{
> + struct replay_insn_list *todo_list = NULL;
> +
> + if (!file_exists(git_path(SEQ_TODO_FILE)))
> + return;
> + read_populate_todo(&todo_list);
> + if (!todo_list->next)
> + remove_sequencer_state(1);
> +}
Not about this patch: I keep on forgetting what the argument to
remove_sequencer_state means. Would it be possible to make it
a flag, which would both make the meaning more obvious and mean
it is easy to support additional flags in the future?
> --- a/t/t3032-merge-recursive-options.sh
> +++ b/t/t3032-merge-recursive-options.sh
> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
> git read-tree --reset -u HEAD &&
> test_must_fail git cherry-pick --no-commit remote &&
> git read-tree --reset -u HEAD &&
> + git cherry-pick --reset &&
> test_must_fail git cherry-pick remote &&
> test_must_fail git update-index --refresh &&
> + git cherry-pick --reset &&
> grep "<<<<<<" text.txt
> '
What is this about? Maybe it would be clearer to change the "git
read-tree ..." to "git reset --hard", so the test assertion would not
rely on new cherry-pick features (and to mention the change in the
commit message!).
Doesn't this point to a risk in the patch? Do you think there might
be scripts out there relying on being able to use "git read-tree
--reset -u HEAD" to clear away a failed cherry-pick before trying
again, and if so, can we do something about it?
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' '
> test_path_is_missing .git/sequencer
> '
>
> -test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
> +test_expect_success 'final commit cleans up sequencer state' '
> pristine_detach initial &&
> test_must_fail git cherry-pick base..picked &&
> - test_path_is_missing .git/sequencer &&
> echo "resolved" >foo &&
> git add foo &&
> git commit &&
> + test_path_is_missing .git/sequencer &&
> {
It would also be nice to check "test_path_is_dir" before the final
commit, so people working on this code in the future know both aspects
of the patch are intentional.
Thanks, I'm glad to see this patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html