* [RFC PATCH] Option to allow cherry-pick to skip empty commits
@ 2017-01-21 13:16 Giuseppe Bilotta
2017-01-22 23:27 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Giuseppe Bilotta @ 2017-01-21 13:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Giuseppe Bilotta
This allows cherry-picking a set of commits, some of which may be
redundant, without stopping to ask for the user intervention.
Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
Documentation/git-cherry-pick.txt | 4 ++++
builtin/revert.c | 1 +
sequencer.c | 45 +++++++++++++++++++++++++++++++--------
sequencer.h | 1 +
4 files changed, 42 insertions(+), 9 deletions(-)
I would like to add to cherry-pick the ability to skip patches. To this
end, I'm working on two options: a general `--skip-empty` option to
handle redundant and empty commits by simply skipping them (no user
intervention), and a `--skip` option as an alternative form to
`--continue` to skip the ongoing (conflicting or empty) commit.
The patch here presents my implementation of the `--skip-empty` option,
including documentation. Comments welcome.
However, it has been some time since I dwelved into git internals and
I'm having issues with the `--skip` option. My idea was to rely on the
existing implementation for `--continue`, and have it run the rollback
instead of the commit. However, I'm finding that after the rollback is
done (I'm using the existing rollback function in sequencer, which calls
`git reset --merge`, but I've also tried a `reset --hard`) the sequencer
does not continue because e.g. if the cherry-pick had stopped due to a
conflict, it still finds unmerged paths, and I can't find a way to tell
the sequencer to just re-read the index from scratch. Opinions welcome
(I can provide my WIP patch to make discussion easier).
On the same note, I've noticed that `git reset` clears CHERRY_PICK_HEAD.
I find this a little confusing, both for humans for the code itself
(since it doesn't really abort an ongoing cherry-pick, yet the sequencer
will think there is no running cherry-pick). This is particularly bad
because `git reset` is one of the strategies proposed to solve conflicts
in cherry-picking. So I was wondering if this was intentional or a side
effect of something else that I might look into cleaning up during this
patch series.
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d35d771fc8..ffced816d6 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -138,6 +138,10 @@ effect to your index in a row.
examine the commit. This option overrides that behavior and
creates an empty commit object. Implies `--allow-empty`.
+--skip-empty::
+ This option causes empty and redundant cherry-picked commits to
+ be skipped without requesting the user intervention.
+
--strategy=<strategy>::
Use the given merge strategy. Should only be used once.
See the MERGE STRATEGIES section in linkgit:git-merge[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index 4ca5b51544..ffdd367f99 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -102,6 +102,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+ OPT_BOOL(0, "skip-empty", &opts->skip_empty, N_("skip redundant, empty commits")),
OPT_END(),
};
options = parse_options_concat(options, cp_extra);
diff --git a/sequencer.c b/sequencer.c
index 9adb7bbf1d..72962fd2fa 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -550,22 +550,32 @@ static int is_original_commit_empty(struct commit *commit)
/*
* Do we run "git commit" with "--allow-empty"?
+ *
+ * Or do we just skip this empty commit?
+ *
+ * Returns 1 if a commit should be done with --allow-empty,
+ * 0 if a commit should be done without --allow-empty,
+ * 2 if no commit should be done at all (skip empty commit)
+ * negative values in case of error
+ *
*/
-static int allow_empty(struct replay_opts *opts, struct commit *commit)
+static int allow_or_skip_empty(struct replay_opts *opts, struct commit *commit)
{
int index_unchanged, empty_commit;
/*
- * Three cases:
+ * Four cases:
*
- * (1) we do not allow empty at all and error out.
+ * (1) we do not allow empty at all and error out;
*
- * (2) we allow ones that were initially empty, but
+ * (2) we skip empty commits altogether;
+ *
+ * (3) we allow ones that were initially empty, but
* forbid the ones that become empty;
*
- * (3) we allow both.
+ * (4) we allow both.
*/
- if (!opts->allow_empty)
+ if (!opts->allow_empty && !opts->skip_empty)
return 0; /* let "git commit" barf as necessary */
index_unchanged = is_index_unchanged();
@@ -574,6 +584,9 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
if (!index_unchanged)
return 0; /* we do not have to say --allow-empty */
+ if (opts->skip_empty)
+ return 2;
+
if (opts->keep_redundant_commits)
return 1;
@@ -612,7 +625,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
const char *base_label, *next_label;
struct commit_message msg = { NULL, NULL, NULL, NULL };
struct strbuf msgbuf = STRBUF_INIT;
- int res, unborn = 0, allow;
+ int res = 0, unborn = 0, allow;
if (opts->no_commit) {
/*
@@ -771,12 +784,13 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
goto leave;
}
- allow = allow_empty(opts, commit);
+ allow = allow_or_skip_empty(opts, commit);
if (allow < 0) {
res = allow;
goto leave;
}
- if (!opts->no_commit)
+ /* allow == 2 means skip this commit */
+ if (allow != 2 && !opts->no_commit)
res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
opts, allow, opts->edit, 0, 0);
@@ -983,6 +997,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
opts->signoff = git_config_bool_or_int(key, value, &error_flag);
else if (!strcmp(key, "options.record-origin"))
opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.skip-empty"))
+ opts->skip_empty = git_config_bool_or_int(key, value, &error_flag);
else if (!strcmp(key, "options.allow-ff"))
opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
else if (!strcmp(key, "options.mainline"))
@@ -1231,6 +1247,8 @@ static int save_opts(struct replay_opts *opts)
res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true");
if (opts->record_origin)
res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true");
+ if (opts->skip_empty)
+ res |= git_config_set_in_file_gently(opts_file, "options.skip-empty", "true");
if (opts->allow_ff)
res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true");
if (opts->mainline) {
@@ -1306,6 +1324,14 @@ int sequencer_continue(struct replay_opts *opts)
if ((res = read_populate_todo(&todo_list, opts)))
goto release_todo_list;
+ /* check if there is something to commit */
+ res = is_index_unchanged();
+ if (res < 0)
+ goto release_todo_list;
+
+ if (res && opts->skip_empty)
+ goto skip_this_commit;
+
/* Verify that the conflict has been resolved */
if (file_exists(git_path_cherry_pick_head()) ||
file_exists(git_path_revert_head())) {
@@ -1317,6 +1343,7 @@ int sequencer_continue(struct replay_opts *opts)
res = error_dirty_index(opts);
goto release_todo_list;
}
+skip_this_commit:
todo_list.current++;
res = pick_commits(&todo_list, opts);
release_todo_list:
diff --git a/sequencer.h b/sequencer.h
index 7a513c576b..c747cfcfc7 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -23,6 +23,7 @@ struct replay_opts {
int allow_empty;
int allow_empty_message;
int keep_redundant_commits;
+ int skip_empty;
int mainline;
--
2.11.0.585.g56041942c3.dirty
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits
2017-01-21 13:16 [RFC PATCH] Option to allow cherry-pick to skip empty commits Giuseppe Bilotta
@ 2017-01-22 23:27 ` Junio C Hamano
2017-01-23 10:18 ` Giuseppe Bilotta
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2017-01-22 23:27 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: git
Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> This allows cherry-picking a set of commits, some of which may be
> redundant, without stopping to ask for the user intervention.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
> Documentation/git-cherry-pick.txt | 4 ++++
> builtin/revert.c | 1 +
> sequencer.c | 45 +++++++++++++++++++++++++++++++--------
> sequencer.h | 1 +
> 4 files changed, 42 insertions(+), 9 deletions(-)
>
>
> I would like to add to cherry-pick the ability to skip patches. To this
> end, I'm working on two options: a general `--skip-empty` option to
> handle redundant and empty commits by simply skipping them (no user
> intervention), and a `--skip` option as an alternative form to
> `--continue` to skip the ongoing (conflicting or empty) commit.
>
> The patch here presents my implementation of the `--skip-empty` option,
> including documentation. Comments welcome.
Sounds like a useful thing to do.
> +static int allow_or_skip_empty(struct replay_opts *opts, struct commit *commit)
> {
> int index_unchanged, empty_commit;
>
> /*
> - * Three cases:
> + * Four cases:
> *
> - * (1) we do not allow empty at all and error out.
> + * (1) we do not allow empty at all and error out;
> *
> - * (2) we allow ones that were initially empty, but
> + * (2) we skip empty commits altogether;
> + *
> + * (3) we allow ones that were initially empty, but
> * forbid the ones that become empty;
> *
> - * (3) we allow both.
> + * (4) we allow both.
> */
The original gave callers the choice to tell two cases (a commit was
empty in the original history, and a commit that was not empty in
the original history turns out to be redundant) apart and handle
them differently. I tend to agree that skipping the former should
be the norm, and also I think it is sensible to drop the latter, and
that is what your updated (2) gives us, I think.
But I would suspect that it would rather be common to have a
deliberately empty commit in the original as a marker in a history
and want to keep that across cherry-picking a series, while wanting
to discard/skip patches that are already applied in an updated base.
Shouldn't that be supported as the fifth case?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits
2017-01-22 23:27 ` Junio C Hamano
@ 2017-01-23 10:18 ` Giuseppe Bilotta
2017-01-23 18:15 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Giuseppe Bilotta @ 2017-01-23 10:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
On Mon, Jan 23, 2017 at 12:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>> +static int allow_or_skip_empty(struct replay_opts *opts, struct commit *commit)
>> {
>> int index_unchanged, empty_commit;
>>
>> /*
>> - * Three cases:
>> + * Four cases:
>> *
>> - * (1) we do not allow empty at all and error out.
>> + * (1) we do not allow empty at all and error out;
>> *
>> - * (2) we allow ones that were initially empty, but
>> + * (2) we skip empty commits altogether;
>> + *
>> + * (3) we allow ones that were initially empty, but
>> * forbid the ones that become empty;
>> *
>> - * (3) we allow both.
>> + * (4) we allow both.
>> */
>
> The original gave callers the choice to tell two cases (a commit was
> empty in the original history, and a commit that was not empty in
> the original history turns out to be redundant) apart and handle
> them differently. I tend to agree that skipping the former should
> be the norm, and also I think it is sensible to drop the latter, and
> that is what your updated (2) gives us, I think.
>
> But I would suspect that it would rather be common to have a
> deliberately empty commit in the original as a marker in a history
> and want to keep that across cherry-picking a series, while wanting
> to discard/skip patches that are already applied in an updated base.
> Shouldn't that be supported as the fifth case?
I was actually wondering about that. I guess the best approach
(symmetric wrt to the --allow) would be to intro introduce
--skip-empty _and_ --skip-redundant, with the former implying the
latter.
By the way, I noticed going over the code that the -allow options are
not stored, so that in case of interruption they will be reset, is
this intentional or a bug?
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits
2017-01-23 10:18 ` Giuseppe Bilotta
@ 2017-01-23 18:15 ` Junio C Hamano
2017-01-23 20:05 ` Giuseppe Bilotta
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2017-01-23 18:15 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: Git List
Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> By the way, I noticed going over the code that the -allow options are
> not stored, so that in case of interruption they will be reset, is
> this intentional or a bug?
I do not know offhand, but given the history of the two commands, I
would guess it was a bug simply overlooked when people bolted "a
series of commits" mode onto these commands that originally worked
only on a single commit.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits
2017-01-23 18:15 ` Junio C Hamano
@ 2017-01-23 20:05 ` Giuseppe Bilotta
2017-01-23 20:10 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Giuseppe Bilotta @ 2017-01-23 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
On Mon, Jan 23, 2017 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> By the way, I noticed going over the code that the -allow options are
>> not stored, so that in case of interruption they will be reset, is
>> this intentional or a bug?
>
> I do not know offhand, but given the history of the two commands, I
> would guess it was a bug simply overlooked when people bolted "a
> series of commits" mode onto these commands that originally worked
> only on a single commit.
It seems like a bug to me. I'll prepare a new series that also fixes
this. I still don't see how to force a complete reread of the index
after running a git reset (which I need for the --skip command), but
maybe I'll add a WIP of what I have for --skip so far and people can
comment on that.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits
2017-01-23 20:05 ` Giuseppe Bilotta
@ 2017-01-23 20:10 ` Junio C Hamano
2017-01-23 22:33 ` Giuseppe Bilotta
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2017-01-23 20:10 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: Git List
Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> ... I still don't see how to force a complete reread of the index
> after running a git reset (which I need for the --skip command), ...
Do you mean discard_index() or discard_cache() followed by
read_index() or read_cache(), or do you mean something more
elaborate?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits
2017-01-23 20:10 ` Junio C Hamano
@ 2017-01-23 22:33 ` Giuseppe Bilotta
0 siblings, 0 replies; 7+ messages in thread
From: Giuseppe Bilotta @ 2017-01-23 22:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
On Mon, Jan 23, 2017 at 9:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> ... I still don't see how to force a complete reread of the index
>> after running a git reset (which I need for the --skip command), ...
>
> Do you mean discard_index() or discard_cache() followed by
> read_index() or read_cache(), or do you mean something more
> elaborate?
Apparently discard_cache() followed by read_cache() is exactly what I
needed. New patchset incoming 8-)
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-01-23 22:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-21 13:16 [RFC PATCH] Option to allow cherry-pick to skip empty commits Giuseppe Bilotta
2017-01-22 23:27 ` Junio C Hamano
2017-01-23 10:18 ` Giuseppe Bilotta
2017-01-23 18:15 ` Junio C Hamano
2017-01-23 20:05 ` Giuseppe Bilotta
2017-01-23 20:10 ` Junio C Hamano
2017-01-23 22:33 ` Giuseppe Bilotta
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.