* [PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits [not found] <Enhance git-rebases flexibiilty in handling empty commits> @ 2012-04-11 20:21 ` Neil Horman 2012-04-11 20:21 ` [PATCH v4 1/4] git-cherry-pick: add allow-empty option Neil Horman ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Neil Horman @ 2012-04-11 20:21 UTC (permalink / raw) To: git; +Cc: Jeff King, Phil Hord, Junio C Hamano, Neil Horman git's ability to handle empty commits is somewhat lacking, especially when preforming a rebase. Nominally empty commits are undesireable entries, the result of commits that are made empty by prior commits covering the same changs. But occasionally, empty commits are useful to developers (e.g. inserting notes into the development history without changing any code along the way). In these cases its desireable to easily preserve empty commits during operations like rebases. This patch series enhances git to do just that. It adds two options to the git-cherry-pick command, --allow-empty, which allows git cherry-pick to preserve an empty commit, even if the fast forward logic isn't applicable during the operation, and --keep-redundant-commits, which allows the user to also keep commits that were made empty via conflict resolution. It also enhances git-rebase to add a --keep-empty option which enables rebases to preserve empty commits. I've tested these operations out myself here and they work well for me Signed-off-by: Neil Horman <nhorman@tuxdriver.com> --- Change notes: Based on version 1 feedback from this list, the following changes have been made V2) * Changed --keep-empty to --allow-empty in the git cherry-pick command * Converted run_git_commit to use argv_array * Updated cherry-pick --allow-empty description in man page * added ignore-if-made-empty option to git-cherry-pick * Added test to test suite to validate the new cherry-pick options * Updated git-rebase man page to be less verbose and more accurate in the description of the keep-empty option * squashed the addition of the keep-empty flag in git-rebase down to one commit from 3 * fixed up coding style in git-rebase script * Optimized detection of empty commits * Only augmented git-rebase-editor message if empty commits are possible V3) * reversed the --ignore-if-empty-logic to by default only keep initially empty commits * replaced --ignore-if-empty with --keep-redundant-commits, to allow empty commits that are made empty via conflict resolution, in addition to commits that were created as empty * reworked is_original_commit_empty to be more efficient and portable * Misc sylistic and spelling cleanups V4) * Reverted the cherry-pick advice changes in V3 based on in-thread discussion * Rewrote my changes to is_original_commit_empty and run_git_commit to not have to fork, making them more efficient. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 1/4] git-cherry-pick: add allow-empty option 2012-04-11 20:21 ` [PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits Neil Horman @ 2012-04-11 20:21 ` Neil Horman 2012-04-11 20:21 ` [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option Neil Horman ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: Neil Horman @ 2012-04-11 20:21 UTC (permalink / raw) To: git; +Cc: Jeff King, Phil Hord, Junio C Hamano, Neil Horman git cherry-pick fails when picking a non-ff commit that is empty. The advice given with the failure is that a git-commit --allow-empty should be issued to explicitly add the empty commit during the cherry pick. This option allows a user to specify before hand that they want to keep the empty commit. This eliminates the need to issue both a cherry pick and a commit operation. Signed-off-by: Neil Horman <nhorman@tuxdriver.com> --- Documentation/git-cherry-pick.txt | 9 +++++++++ builtin/revert.c | 2 ++ sequencer.c | 7 +++++-- sequencer.h | 1 + 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index fed5097..730237a 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -103,6 +103,15 @@ effect to your index in a row. cherry-pick'ed commit, then a fast forward to this commit will be performed. +--allow-empty:: + By default, cherry-picking an empty commit will fail, + indicating that an explicit invocation of `git commit + --allow-empty` is required. This option overrides that + behavior, allowing empty commits to be preserved automatically + in a cherry-pick. Note that when "--ff" is in effect, empty + commits that meet the "fast-forward" requirement will be kept + even without this option. + --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 e6840f2..06b00e6 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -114,12 +114,14 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) OPT_END(), OPT_END(), OPT_END(), + OPT_END(), }; if (opts->action == REPLAY_PICK) { struct option cp_extra[] = { OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"), OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"), + OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve empty commits"), OPT_END(), }; if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra)) diff --git a/sequencer.c b/sequencer.c index a37846a..71929ba 100644 --- a/sequencer.c +++ b/sequencer.c @@ -260,8 +260,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next, */ static int run_git_commit(const char *defmsg, struct replay_opts *opts) { - /* 6 is max possible length of our args array including NULL */ - const char *args[6]; + /* 7 is max possible length of our args array including NULL */ + const char *args[7]; int i = 0; args[i++] = "commit"; @@ -272,6 +272,9 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts) args[i++] = "-F"; args[i++] = defmsg; } + if (opts->allow_empty) + args[i++] = "--allow-empty"; + args[i] = NULL; return run_command_v_opt(args, RUN_GIT_CMD); diff --git a/sequencer.h b/sequencer.h index bb4b138..e2cd725 100644 --- a/sequencer.h +++ b/sequencer.h @@ -29,6 +29,7 @@ struct replay_opts { int signoff; int allow_ff; int allow_rerere_auto; + int allow_empty; int mainline; -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option 2012-04-11 20:21 ` [PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits Neil Horman 2012-04-11 20:21 ` [PATCH v4 1/4] git-cherry-pick: add allow-empty option Neil Horman @ 2012-04-11 20:21 ` Neil Horman 2012-04-11 20:47 ` Junio C Hamano 2012-04-11 20:57 ` Junio C Hamano 2012-04-11 20:21 ` [PATCH v4 3/4] git-cherry-pick: Add test to validate new options Neil Horman 2012-04-11 20:21 ` [PATCH v4 4/4] git-rebase: add keep_empty flag Neil Horman 3 siblings, 2 replies; 16+ messages in thread From: Neil Horman @ 2012-04-11 20:21 UTC (permalink / raw) To: git; +Cc: Jeff King, Phil Hord, Junio C Hamano, Neil Horman The git-cherry-pick --allow-empty command by default only preserves empty commits that were originally empty, i.e only those commits for which <commit>^{tree} and <commit>^^{tree} are equal. By default commits which are non-empty, but were made empty by the inclusion of a prior commit on the current history are filtered out. This option allows us to override that behavior and include redundant commits as empty commits in the change history. Signed-off-by: Neil Horman <nhorman@tuxdriver.com> --- Documentation/git-cherry-pick.txt | 12 +++++- builtin/revert.c | 8 +++- sequencer.c | 91 +++++++++++++++++++++++++++++++----- sequencer.h | 1 + 4 files changed, 97 insertions(+), 15 deletions(-) diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 730237a..f96b8c5 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -110,7 +110,17 @@ effect to your index in a row. behavior, allowing empty commits to be preserved automatically in a cherry-pick. Note that when "--ff" is in effect, empty commits that meet the "fast-forward" requirement will be kept - even without this option. + even without this option. Note also, that use of this option only + keeps commits that were initially empty (i.e. where for commit C + C^{tree} and C^^{tree} are equal). Commits which are made empty due to + a previous commit are ignored. To force the inclusion of those commits + use `--keep-redundant-commits` + +--keep-redundant-commits:: + If a commit being cherry picked duplicates a commit already in the + current history, it will result in an empty changeset. By default these + redundant commits are ignored. This option overrides that behavior and + creates an empty commit object. Implies `--allow-empty` --strategy=<strategy>:: Use the given merge strategy. Should only be used once. diff --git a/builtin/revert.c b/builtin/revert.c index 06b00e6..4f0d979 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) OPT_END(), OPT_END(), OPT_END(), + OPT_END(), }; if (opts->action == REPLAY_PICK) { struct option cp_extra[] = { OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"), OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"), - OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve empty commits"), + OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve initially empty commits"), + OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"), OPT_END(), }; if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra)) @@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) "--abort", rollback, NULL); + /* keep_if_made_empty implies allow_empty */ + if (opts->keep_if_made_empty) + opts->allow_empty = 1; + /* Set the subcommand */ if (remove_state) opts->subcommand = REPLAY_REMOVE_STATE; diff --git a/sequencer.c b/sequencer.c index 71929ba..442f364 100644 --- a/sequencer.c +++ b/sequencer.c @@ -13,6 +13,7 @@ #include "rerere.h" #include "merge-recursive.h" #include "refs.h" +#include "argv-array.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next, * If we are revert, or if our cherry-pick results in a hand merge, * we had better say that the current user is responsible for that. */ -static int run_git_commit(const char *defmsg, struct replay_opts *opts) +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty) { - /* 7 is max possible length of our args array including NULL */ - const char *args[7]; - int i = 0; + struct argv_array array; + int rc; + + if (!empty && !opts->keep_if_made_empty) { + unsigned char head_sha1[20]; + struct commit *head_commit; + int need_free = 0; + + resolve_ref_unsafe("HEAD", head_sha1, 1, NULL); + head_commit = lookup_commit(head_sha1); + if (parse_commit(head_commit) < 0) + return error(_("could not parse commit %s\n"), + sha1_to_hex(head_commit->object.sha1)); + + if (!active_cache_tree) { + active_cache_tree = cache_tree(); + need_free = 1; + } + + if (!cache_tree_fully_valid(active_cache_tree)) + cache_tree_update(active_cache_tree, active_cache, + active_nr, 0); + + rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1); + + if (need_free) + cache_tree_free(&active_cache_tree); + + if (rc) + /* + * The head tree and the parent tree match + * meaning the commit is empty. Since it wasn't created + * empty (based on the previous test), we can conclude + * the commit has been made redundant. Since we don't + * want to keep redundant commits, just skip this one + */ + return 0; + } + + argv_array_init(&array); + argv_array_push(&array, "commit"); + argv_array_push(&array, "-n"); - args[i++] = "commit"; - args[i++] = "-n"; if (opts->signoff) - args[i++] = "-s"; + argv_array_push(&array, "-s"); if (!opts->edit) { - args[i++] = "-F"; - args[i++] = defmsg; + argv_array_push(&array, "-F"); + argv_array_push(&array, defmsg); } + if (opts->allow_empty) - args[i++] = "--allow-empty"; + argv_array_push(&array, "--allow-empty"); + + + rc = run_command_v_opt(array.argv, RUN_GIT_CMD); + argv_array_clear(&array); + return rc; +} - args[i] = NULL; +static int is_original_commit_empty(struct commit *commit) +{ + const unsigned char *ptree_sha1; + + if (parse_commit(commit)) + return error(_("Could not parse commit %s\n"), + sha1_to_hex(commit->object.sha1)); + if (commit->parents) { + struct commit *parent = commit->parents->item; + if (parse_commit(parent)) + return error(_("Could not parse parent commit %s\n"), + sha1_to_hex(parent->object.sha1)); + ptree_sha1 = parent->tree->object.sha1; + } else { + ptree_sha1 = EMPTY_TREE_SHA1_BIN; /* commit is root */ + } - return run_command_v_opt(args, RUN_GIT_CMD); + return !hashcmp(ptree_sha1, commit->tree->object.sha1); } static int do_pick_commit(struct commit *commit, struct replay_opts *opts) @@ -289,6 +349,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) char *defmsg = NULL; struct strbuf msgbuf = STRBUF_INIT; int res; + int empty_commit; if (opts->no_commit) { /* @@ -414,6 +475,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) free_commit_list(remotes); } + empty_commit = is_original_commit_empty(commit); + if (empty_commit < 0) + return empty_commit; + /* * If the merge was clean or if it failed due to conflict, we write * CHERRY_PICK_HEAD for the subsequent invocation of commit to use. @@ -435,7 +500,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) rerere(opts->allow_rerere_auto); } else { if (!opts->no_commit) - res = run_git_commit(defmsg, opts); + res = run_git_commit(defmsg, opts, empty_commit); } free_message(&msg); diff --git a/sequencer.h b/sequencer.h index e2cd725..862a79a 100644 --- a/sequencer.h +++ b/sequencer.h @@ -30,6 +30,7 @@ struct replay_opts { int allow_ff; int allow_rerere_auto; int allow_empty; + int keep_if_made_empty; int mainline; -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option 2012-04-11 20:21 ` [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option Neil Horman @ 2012-04-11 20:47 ` Junio C Hamano 2012-04-11 23:35 ` Neil Horman 2012-04-11 20:57 ` Junio C Hamano 1 sibling, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2012-04-11 20:47 UTC (permalink / raw) To: Neil Horman; +Cc: git, Jeff King, Phil Hord You inherited the following from your previous round. Applying: git-cherry-pick: Add keep-redundant-commits option /srv/project/git/git.git/.git/rebase-apply/patch:115: space before tab in indent. * The head tree and the parent tree match /srv/project/git/git.git/.git/rebase-apply/patch:116: space before tab in indent. * meaning the commit is empty. Since it wasn't created /srv/project/git/git.git/.git/rebase-apply/patch:117: space before tab in indent. * empty (based on the previous test), we can conclude /srv/project/git/git.git/.git/rebase-apply/patch:118: space before tab in indent. * the commit has been made redundant. Since we don't /srv/project/git/git.git/.git/rebase-apply/patch:119: space before tab in indent. * want to keep redundant commits, just skip this one warning: squelched 3 whitespace errors fatal: 8 lines add whitespace errors. Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 git-cherry-pick: Add keep-redundant-commits option ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option 2012-04-11 20:47 ` Junio C Hamano @ 2012-04-11 23:35 ` Neil Horman 0 siblings, 0 replies; 16+ messages in thread From: Neil Horman @ 2012-04-11 23:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Phil Hord On Wed, Apr 11, 2012 at 01:47:48PM -0700, Junio C Hamano wrote: > You inherited the following from your previous round. > > Applying: git-cherry-pick: Add keep-redundant-commits option > /srv/project/git/git.git/.git/rebase-apply/patch:115: space before tab in indent. > * The head tree and the parent tree match > /srv/project/git/git.git/.git/rebase-apply/patch:116: space before tab in indent. > * meaning the commit is empty. Since it wasn't created > /srv/project/git/git.git/.git/rebase-apply/patch:117: space before tab in indent. > * empty (based on the previous test), we can conclude > /srv/project/git/git.git/.git/rebase-apply/patch:118: space before tab in indent. > * the commit has been made redundant. Since we don't > /srv/project/git/git.git/.git/rebase-apply/patch:119: space before tab in indent. > * want to keep redundant commits, just skip this one > warning: squelched 3 whitespace errors > fatal: 8 lines add whitespace errors. > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. > Patch failed at 0001 git-cherry-pick: Add keep-redundant-commits option > Grr, thanks, forgot to run checkpatch Neil ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option 2012-04-11 20:21 ` [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option Neil Horman 2012-04-11 20:47 ` Junio C Hamano @ 2012-04-11 20:57 ` Junio C Hamano 2012-04-11 23:55 ` Neil Horman 1 sibling, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2012-04-11 20:57 UTC (permalink / raw) To: Neil Horman; +Cc: git, Jeff King, Phil Hord Neil Horman <nhorman@tuxdriver.com> writes: > The git-cherry-pick --allow-empty command by default only preserves empty > commits that were originally empty, i.e only those commits for which > <commit>^{tree} and <commit>^^{tree} are equal. By default commits which are > non-empty, but were made empty by the inclusion of a prior commit on the current > history are filtered out. This option allows us to override that behavior and > include redundant commits as empty commits in the change history. > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > --- > Documentation/git-cherry-pick.txt | 12 +++++- > builtin/revert.c | 8 +++- > sequencer.c | 91 +++++++++++++++++++++++++++++++----- > sequencer.h | 1 + > 4 files changed, 97 insertions(+), 15 deletions(-) > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > index 730237a..f96b8c5 100644 > --- a/Documentation/git-cherry-pick.txt > +++ b/Documentation/git-cherry-pick.txt > @@ -110,7 +110,17 @@ effect to your index in a row. > behavior, allowing empty commits to be preserved automatically > in a cherry-pick. Note that when "--ff" is in effect, empty > commits that meet the "fast-forward" requirement will be kept > - even without this option. > + even without this option. Note also, that use of this option only > + keeps commits that were initially empty (i.e. where for commit C > + C^{tree} and C^^{tree} are equal). Commits which are made empty due to It is OK to be technical in the log message, but I think commits that were initially empty (i.e. the commit recorded the same tree as its parent) would be far easier to read in the documentation meant for the end users. > + a previous commit are ignored. To force the inclusion of those commits > + use `--keep-redundant-commits` And end the sentence with a full-stop. > +--keep-redundant-commits:: > + If a commit being cherry picked duplicates a commit already in the > + current history, it will result in an empty changeset. By default these > + redundant commits are ignored. This option overrides that behavior and > + creates an empty commit object. Implies `--allow-empty` Likewise. > diff --git a/builtin/revert.c b/builtin/revert.c > index 06b00e6..4f0d979 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) > OPT_END(), > OPT_END(), > OPT_END(), > + OPT_END(), > }; > > if (opts->action == REPLAY_PICK) { > struct option cp_extra[] = { > OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"), > OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"), > - OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve empty commits"), > + OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve initially empty commits"), > + OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"), > OPT_END(), > }; > if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra)) > @@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) > "--abort", rollback, > NULL); > > + /* keep_if_made_empty implies allow_empty */ > + if (opts->keep_if_made_empty) > + opts->allow_empty = 1; > + > /* Set the subcommand */ > if (remove_state) > opts->subcommand = REPLAY_REMOVE_STATE; > diff --git a/sequencer.c b/sequencer.c > index 71929ba..442f364 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -13,6 +13,7 @@ > #include "rerere.h" > #include "merge-recursive.h" > #include "refs.h" > +#include "argv-array.h" > > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > @@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next, > * If we are revert, or if our cherry-pick results in a hand merge, > * we had better say that the current user is responsible for that. > */ > -static int run_git_commit(const char *defmsg, struct replay_opts *opts) > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty) > { > - /* 7 is max possible length of our args array including NULL */ > - const char *args[7]; > - int i = 0; > + struct argv_array array; > + int rc; > + > + if (!empty && !opts->keep_if_made_empty) { > + unsigned char head_sha1[20]; > + struct commit *head_commit; > + int need_free = 0; > + > + resolve_ref_unsafe("HEAD", head_sha1, 1, NULL); > + head_commit = lookup_commit(head_sha1); No error checking whatsoever? HEAD might be pointing at a branch that hasn't been born, for example. > + if (parse_commit(head_commit) < 0) > + return error(_("could not parse commit %s\n"), > + sha1_to_hex(head_commit->object.sha1)); > + > + if (!active_cache_tree) { > + active_cache_tree = cache_tree(); > + need_free = 1; > + } I think this is wrong. If for whatever reason the process does not have the index it is about to tell "git commit" to use to make a commit in-core, it should first be doing read_cache(). > + > + if (!cache_tree_fully_valid(active_cache_tree)) > + cache_tree_update(active_cache_tree, active_cache, > + active_nr, 0); I do not recall offhand if cache_tree_update() can give you an error, but if it does, it should be checked here. > + rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1); > + > + if (need_free) > + cache_tree_free(&active_cache_tree); and I don't see a need for nuking the cache tree here, unless you are discarding the_index as well. > + if (rc) > + /* > + * The head tree and the parent tree match > + * meaning the commit is empty. Since it wasn't created > + * empty (based on the previous test), we can conclude > + * the commit has been made redundant. Since we don't > + * want to keep redundant commits, just skip this one > + */ > + return 0; > + } > + > + argv_array_init(&array); > + argv_array_push(&array, "commit"); > + argv_array_push(&array, "-n"); > > - args[i++] = "commit"; > - args[i++] = "-n"; > if (opts->signoff) > - args[i++] = "-s"; > + argv_array_push(&array, "-s"); > if (!opts->edit) { > - args[i++] = "-F"; > - args[i++] = defmsg; > + argv_array_push(&array, "-F"); > + argv_array_push(&array, defmsg); > } > + > if (opts->allow_empty) > - args[i++] = "--allow-empty"; > + argv_array_push(&array, "--allow-empty"); If --keep-if-made-empty is not given but --allow-empty was, it is fine to give --allow-empty here, but otherwise, it logically is iffy and is likely to become a cause of future bugs to pass --allow-empty to "git commit", even though the earlier check _ought_ to catch problematic cases, no? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option 2012-04-11 20:57 ` Junio C Hamano @ 2012-04-11 23:55 ` Neil Horman 2012-04-12 3:15 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Neil Horman @ 2012-04-11 23:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Phil Hord On Wed, Apr 11, 2012 at 01:57:24PM -0700, Junio C Hamano wrote: > Neil Horman <nhorman@tuxdriver.com> writes: > > > The git-cherry-pick --allow-empty command by default only preserves empty > > commits that were originally empty, i.e only those commits for which > > <commit>^{tree} and <commit>^^{tree} are equal. By default commits which are > > non-empty, but were made empty by the inclusion of a prior commit on the current > > history are filtered out. This option allows us to override that behavior and > > include redundant commits as empty commits in the change history. > > > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > > --- > > Documentation/git-cherry-pick.txt | 12 +++++- > > builtin/revert.c | 8 +++- > > sequencer.c | 91 +++++++++++++++++++++++++++++++----- > > sequencer.h | 1 + > > 4 files changed, 97 insertions(+), 15 deletions(-) > > > > diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt > > index 730237a..f96b8c5 100644 > > --- a/Documentation/git-cherry-pick.txt > > +++ b/Documentation/git-cherry-pick.txt > > @@ -110,7 +110,17 @@ effect to your index in a row. > > behavior, allowing empty commits to be preserved automatically > > in a cherry-pick. Note that when "--ff" is in effect, empty > > commits that meet the "fast-forward" requirement will be kept > > - even without this option. > > + even without this option. Note also, that use of this option only > > + keeps commits that were initially empty (i.e. where for commit C > > + C^{tree} and C^^{tree} are equal). Commits which are made empty due to > > It is OK to be technical in the log message, but I think > > commits that were initially empty (i.e. the commit recorded the > same tree as its parent) > > would be far easier to read in the documentation meant for the end users. > > > + a previous commit are ignored. To force the inclusion of those commits > > + use `--keep-redundant-commits` > > And end the sentence with a full-stop. > Copy that. > > +--keep-redundant-commits:: > > + If a commit being cherry picked duplicates a commit already in the > > + current history, it will result in an empty changeset. By default these > > + redundant commits are ignored. This option overrides that behavior and > > + creates an empty commit object. Implies `--allow-empty` > > Likewise. > ACK > > diff --git a/builtin/revert.c b/builtin/revert.c > > index 06b00e6..4f0d979 100644 > > --- a/builtin/revert.c > > +++ b/builtin/revert.c > > @@ -115,13 +115,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) > > OPT_END(), > > OPT_END(), > > OPT_END(), > > + OPT_END(), > > }; > > > > if (opts->action == REPLAY_PICK) { > > struct option cp_extra[] = { > > OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"), > > OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"), > > - OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve empty commits"), > > + OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, "preserve initially empty commits"), > > + OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"), > > OPT_END(), > > }; > > if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra)) > > @@ -139,6 +141,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) > > "--abort", rollback, > > NULL); > > > > + /* keep_if_made_empty implies allow_empty */ > > + if (opts->keep_if_made_empty) > > + opts->allow_empty = 1; > > + > > /* Set the subcommand */ > > if (remove_state) > > opts->subcommand = REPLAY_REMOVE_STATE; > > diff --git a/sequencer.c b/sequencer.c > > index 71929ba..442f364 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -13,6 +13,7 @@ > > #include "rerere.h" > > #include "merge-recursive.h" > > #include "refs.h" > > +#include "argv-array.h" > > > > #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" > > > > @@ -258,26 +259,85 @@ static int do_recursive_merge(struct commit *base, struct commit *next, > > * If we are revert, or if our cherry-pick results in a hand merge, > > * we had better say that the current user is responsible for that. > > */ > > -static int run_git_commit(const char *defmsg, struct replay_opts *opts) > > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty) > > { > > - /* 7 is max possible length of our args array including NULL */ > > - const char *args[7]; > > - int i = 0; > > + struct argv_array array; > > + int rc; > > + > > + if (!empty && !opts->keep_if_made_empty) { > > + unsigned char head_sha1[20]; > > + struct commit *head_commit; > > + int need_free = 0; > > + > > + resolve_ref_unsafe("HEAD", head_sha1, 1, NULL); > > + head_commit = lookup_commit(head_sha1); > > No error checking whatsoever? HEAD might be pointing at a branch that > hasn't been born, for example. > Sorry, I had presumed that HEAD could be guaranteed to be good here, given that we validate it in rollback_single_pick via read_ref_full. But I can add additional checking here. > > + if (parse_commit(head_commit) < 0) > > + return error(_("could not parse commit %s\n"), > > + sha1_to_hex(head_commit->object.sha1)); > > + > > + if (!active_cache_tree) { > > + active_cache_tree = cache_tree(); > > + need_free = 1; > > + } > > I think this is wrong. If for whatever reason the process does not have > the index it is about to tell "git commit" to use to make a commit in-core, > it should first be doing read_cache(). I would have thought so too, and perhaps it should, but it doesn't seem to be the case. Initially I hadn't added this code, but during testing I received a segfault indicating that the_index.cache_tree was NULL. I thought it should have been filled in via the calls to read_cache in do_recursive_merge or fast_forward_to, but it didn't seem to be the case. I added a read_cache call right before my call to cache_tree_update, but still the_index.cache_tree remained NULL. Looking at the read_cache code, it appears that cache_tree is only allocated if a CACHE_EXT_TREE is found when parsing the index. Not sure why I wouldn't have one, but certainly it didn't seem I did. The above code fixed it, which I lifted directly from write_tree_from_memory() > > + > > + if (!cache_tree_fully_valid(active_cache_tree)) > > + cache_tree_update(active_cache_tree, active_cache, > > + active_nr, 0); > > I do not recall offhand if cache_tree_update() can give you an error, but > if it does, it should be checked here. > It can, and I'll add checking there, but I hit the segfault above prior to getting any error checknig done. > > + rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1); > > + > > + if (need_free) > > + cache_tree_free(&active_cache_tree); > > and I don't see a need for nuking the cache tree here, unless you are > discarding the_index as well. > Just balancing the allocation above with a free. I didn't see a need for it any longer, given that we were shortly going to call git commit and then exit this process. Is it safe to leave hanging around? > > + if (rc) > > + /* > > + * The head tree and the parent tree match > > + * meaning the commit is empty. Since it wasn't created > > + * empty (based on the previous test), we can conclude > > + * the commit has been made redundant. Since we don't > > + * want to keep redundant commits, just skip this one > > + */ > > + return 0; > > + } > > + > > + argv_array_init(&array); > > + argv_array_push(&array, "commit"); > > + argv_array_push(&array, "-n"); > > > > - args[i++] = "commit"; > > - args[i++] = "-n"; > > if (opts->signoff) > > - args[i++] = "-s"; > > + argv_array_push(&array, "-s"); > > if (!opts->edit) { > > - args[i++] = "-F"; > > - args[i++] = defmsg; > > + argv_array_push(&array, "-F"); > > + argv_array_push(&array, defmsg); > > } > > + > > if (opts->allow_empty) > > - args[i++] = "--allow-empty"; > > + argv_array_push(&array, "--allow-empty"); > > If --keep-if-made-empty is not given but --allow-empty was, it is fine to > give --allow-empty here, but otherwise, it logically is iffy and is likely > to become a cause of future bugs to pass --allow-empty to "git commit", > even though the earlier check _ought_ to catch problematic cases, no? > Not sure I follow your reasoning here. We need to pass allow-empty to git commit here if git cherry-pick set either allow-empty or keep-redundant-commits (the latter setting opts->empty), Can you give an example of what might be problematic in the future? Neil ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option 2012-04-11 23:55 ` Neil Horman @ 2012-04-12 3:15 ` Junio C Hamano 2012-04-12 10:49 ` Neil Horman 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2012-04-12 3:15 UTC (permalink / raw) To: Neil Horman; +Cc: git, Jeff King, Phil Hord Neil Horman <nhorman@tuxdriver.com> writes: >> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts) >> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty) >> > { >> > - /* 7 is max possible length of our args array including NULL */ >> > - const char *args[7]; >> > - int i = 0; >> > + struct argv_array array; >> > + int rc; >> > + >> > + if (!empty && !opts->keep_if_made_empty) { >> > + unsigned char head_sha1[20]; >> > + struct commit *head_commit; >> > + int need_free = 0; >> > + >> > + resolve_ref_unsafe("HEAD", head_sha1, 1, NULL); >> > + head_commit = lookup_commit(head_sha1); >> >> No error checking whatsoever? HEAD might be pointing at a branch that >> hasn't been born, for example. >> > Sorry, I had presumed that HEAD could be guaranteed to be good here, given that > we validate it in rollback_single_pick via read_ref_full. But I can add > additional checking here. As long as it is clear that we always have "HEAD" here, it is not strictly necessary, but we'd probably feel better, especially now that the function is (for some reason) made into a global one and may gain other new callers outside this file scope. >> > if (opts->allow_empty) >> > - args[i++] = "--allow-empty"; >> > + argv_array_push(&array, "--allow-empty"); >> >> If --keep-if-made-empty is not given but --allow-empty was, it is fine to >> give --allow-empty here, but otherwise, it logically is iffy and is likely >> to become a cause of future bugs to pass --allow-empty to "git commit", >> even though the earlier check _ought_ to catch problematic cases, no? >> > Not sure I follow your reasoning here. We need to pass allow-empty to git > commit here if git cherry-pick set either allow-empty or keep-redundant-commits > (the latter setting opts->empty), Can you give an example of what might be > problematic in the future? Thinking about it again, I think this part of your patch is correct. We may pass an index that yields the same tree as "HEAD" if the original was empty, or the original was not empty but the merge found the commit unneeded. In either case, if "--keep-unnecessary" or "--allow-empty" was given, we want to record the empty commit, and opts->allow_empty is set when either option was given from the command line. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option 2012-04-12 3:15 ` Junio C Hamano @ 2012-04-12 10:49 ` Neil Horman 0 siblings, 0 replies; 16+ messages in thread From: Neil Horman @ 2012-04-12 10:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Phil Hord On Wed, Apr 11, 2012 at 08:15:15PM -0700, Junio C Hamano wrote: > Neil Horman <nhorman@tuxdriver.com> writes: > > >> > -static int run_git_commit(const char *defmsg, struct replay_opts *opts) > >> > +int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty) > >> > { > >> > - /* 7 is max possible length of our args array including NULL */ > >> > - const char *args[7]; > >> > - int i = 0; > >> > + struct argv_array array; > >> > + int rc; > >> > + > >> > + if (!empty && !opts->keep_if_made_empty) { > >> > + unsigned char head_sha1[20]; > >> > + struct commit *head_commit; > >> > + int need_free = 0; > >> > + > >> > + resolve_ref_unsafe("HEAD", head_sha1, 1, NULL); > >> > + head_commit = lookup_commit(head_sha1); > >> > >> No error checking whatsoever? HEAD might be pointing at a branch that > >> hasn't been born, for example. > >> > > Sorry, I had presumed that HEAD could be guaranteed to be good here, given that > > we validate it in rollback_single_pick via read_ref_full. But I can add > > additional checking here. > > As long as it is clear that we always have "HEAD" here, it is not strictly > necessary, but we'd probably feel better, especially now that the function > is (for some reason) made into a global one and may gain other new callers > outside this file scope. > Gah! Thanks for that, its not meant to be static. I made that global so that gdb could see it and set a breakpoint on it, while I was figuring out the segfault I was getting when active_cache_tree was NULL. I'll fix that. > >> > if (opts->allow_empty) > >> > - args[i++] = "--allow-empty"; > >> > + argv_array_push(&array, "--allow-empty"); > >> > >> If --keep-if-made-empty is not given but --allow-empty was, it is fine to > >> give --allow-empty here, but otherwise, it logically is iffy and is likely > >> to become a cause of future bugs to pass --allow-empty to "git commit", > >> even though the earlier check _ought_ to catch problematic cases, no? > >> > > Not sure I follow your reasoning here. We need to pass allow-empty to git > > commit here if git cherry-pick set either allow-empty or keep-redundant-commits > > (the latter setting opts->empty), Can you give an example of what might be > > problematic in the future? > > Thinking about it again, I think this part of your patch is correct. > > We may pass an index that yields the same tree as "HEAD" if the original > was empty, or the original was not empty but the merge found the commit > unneeded. In either case, if "--keep-unnecessary" or "--allow-empty" was > given, we want to record the empty commit, and opts->allow_empty is set > when either option was given from the command line. > Yes, exactly. Thanks. > Thanks. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 3/4] git-cherry-pick: Add test to validate new options 2012-04-11 20:21 ` [PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits Neil Horman 2012-04-11 20:21 ` [PATCH v4 1/4] git-cherry-pick: add allow-empty option Neil Horman 2012-04-11 20:21 ` [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option Neil Horman @ 2012-04-11 20:21 ` Neil Horman 2012-04-11 21:06 ` Junio C Hamano 2012-04-11 20:21 ` [PATCH v4 4/4] git-rebase: add keep_empty flag Neil Horman 3 siblings, 1 reply; 16+ messages in thread From: Neil Horman @ 2012-04-11 20:21 UTC (permalink / raw) To: git; +Cc: Jeff King, Phil Hord, Junio C Hamano, Neil Horman Since we've added the --allow-empty and --keep-redundant-commits options to git cherry-pick we should also add a test to ensure that its working properly Signed-off-by: Neil Horman <nhorman@tuxdriver.com> --- t/t3505-cherry-pick-empty.sh | 31 ++++++++++++++++++++++++++++++- 1 files changed, 30 insertions(+), 1 deletions(-) diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh index c10b28c..9d419ae 100755 --- a/t/t3505-cherry-pick-empty.sh +++ b/t/t3505-cherry-pick-empty.sh @@ -18,7 +18,12 @@ test_expect_success setup ' echo third >> file1 && git add file1 && test_tick && - git commit --allow-empty-message -m "" + git commit --allow-empty-message -m "" && + + git checkout master && + git checkout -b empty-branch2 && + test_tick && + git commit --allow-empty -m "empty" ' @@ -48,4 +53,28 @@ test_expect_success 'index lockfile was removed' ' ' +test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' ' + git checkout master && + echo fourth >> file2 && + git add file2 && + git commit -m "fourth" && { + git cherry-pick empty-branch2 + test "$?" = 1 + } +' + +test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' ' + git checkout master && { + git cherry-pick --allow-empty empty-branch2 + test "$?" = 0 + } +' + +test_expect_success 'cherry pick with --keep-redundant-commits' ' + git checkout master && { + git cherry-pick --keep-redundant-commits HEAD^ + test "$?" = 0 + } +' + test_done -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 3/4] git-cherry-pick: Add test to validate new options 2012-04-11 20:21 ` [PATCH v4 3/4] git-cherry-pick: Add test to validate new options Neil Horman @ 2012-04-11 21:06 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2012-04-11 21:06 UTC (permalink / raw) To: Neil Horman; +Cc: git, Jeff King, Phil Hord Neil Horman <nhorman@tuxdriver.com> writes: > Since we've added the --allow-empty and --keep-redundant-commits > options to git cherry-pick we should also add a test to ensure that its working > properly Missing full-stop at the end of the sentence > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > +test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' ' > + git checkout master && > + echo fourth >> file2 && > + git add file2 && > + git commit -m "fourth" && { > + git cherry-pick empty-branch2 > + test "$?" = 1 > + } > +' Do we _deeply_ care that it fails with status 1, or should this be test_must_fail? > +test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' ' > + git checkout master && { > + git cherry-pick --allow-empty empty-branch2 > + test "$?" = 0 > + } > +' Lose the 'test "$?" = 0' here. > +test_expect_success 'cherry pick with --keep-redundant-commits' ' > + git checkout master && { > + git cherry-pick --keep-redundant-commits HEAD^ > + test "$?" = 0 Likewise. > + } > +' > + > test_done Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 4/4] git-rebase: add keep_empty flag 2012-04-11 20:21 ` [PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits Neil Horman ` (2 preceding siblings ...) 2012-04-11 20:21 ` [PATCH v4 3/4] git-cherry-pick: Add test to validate new options Neil Horman @ 2012-04-11 20:21 ` Neil Horman 2012-04-11 21:08 ` Junio C Hamano 3 siblings, 1 reply; 16+ messages in thread From: Neil Horman @ 2012-04-11 20:21 UTC (permalink / raw) To: git; +Cc: Jeff King, Phil Hord, Junio C Hamano, Neil Horman Add a command line switch to git-rebase to allow a user the ability to specify that they want to keep any commits in a series that are empty. When git-rebase's type is am, then this option will automatically keep any commit that has a tree object identical to its parent. This patch changes the default behavior of interactive rebases as well. With this patch, git-rebase -i will produce a revision set passed to git-revision-editor, in which empty commits are commented out. Empty commits may be kept manually by uncommenting them. If the new --keep-empty option is used in an interactive rebase the empty commits will automatically all be uncommented in the editor. Signed-off-by: Neil Horman <nhorman@tuxdriver.com> --- Documentation/git-rebase.txt | 4 ++++ git-rebase--am.sh | 19 ++++++++++++++----- git-rebase--interactive.sh | 35 ++++++++++++++++++++++++++++++++--- git-rebase.sh | 5 +++++ 4 files changed, 55 insertions(+), 8 deletions(-) diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 504945c..131c35d 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -238,6 +238,10 @@ leave out at most one of A and B, in which case it defaults to HEAD. will be reset to where it was when the rebase operation was started. +--keep-empty:: + Keep the commits that do not change anything from its + parents in the result. + --skip:: Restart the rebasing process by skipping the current patch. diff --git a/git-rebase--am.sh b/git-rebase--am.sh index c815a24..040289c 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -20,11 +20,20 @@ esac test -n "$rebase_root" && root_flag=--root -git format-patch -k --stdout --full-index --ignore-if-in-upstream \ - --src-prefix=a/ --dst-prefix=b/ \ - --no-renames $root_flag "$revisions" | -git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" && -move_to_original_branch +if test -n "$keep_empty" +then + # we have to do this the hard way. git format-patch completely squashes + # empty commits and even if it didn't the format doesn't really lend + # itself well to recording empty patches. fortunately, cherry-pick + # makes this easy + git cherry-pick --allow-empty "$revisions" +else + git format-patch -k --stdout --full-index --ignore-if-in-upstream \ + --src-prefix=a/ --dst-prefix=b/ \ + --no-renames $root_flag "$revisions" | + git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" +fi && move_to_original_branch + ret=$? test 0 != $ret -a -d "$state_dir" && write_basic_state exit $ret diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 5812222..597d60a 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -167,6 +167,12 @@ has_action () { sane_grep '^[^#]' "$1" >/dev/null } +is_empty_commit() { + ptree=$(git rev-parse "$1"^{tree}) + pptree=$(git rev-parse "$1"^^{tree}) + return $(test "$ptree" = "$pptree") +} + # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and # GIT_AUTHOR_DATE exported from the current environment. do_with_author () { @@ -191,12 +197,18 @@ git_sequence_editor () { pick_one () { ff=--ff + + if is_empty_commit $@ + then + empty_args="--allow-empty" + fi + case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac case "$force_rebase" in '') ;; ?*) ff= ;; esac output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1" test -d "$rewritten" && pick_one_preserving_merges "$@" && return - output git cherry-pick $ff "$@" + output git cherry-pick $empty_args $ff "$@" } pick_one_preserving_merges () { @@ -780,9 +792,17 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \ sed -n "s/^>//p" | while read -r shortsha1 rest do + + if test -z "$keep_empty" && is_empty_commit $shortsha1 + then + comment_out="# pick" + else + comment_out="pick" + fi + if test t != "$preserve_merges" then - printf '%s\n' "pick $shortsha1 $rest" >> "$todo" + printf '%s\n' "$comment_out $shortsha1 $rest" >> "$todo" else sha1=$(git rev-parse $shortsha1) if test -z "$rebase_root" @@ -801,7 +821,7 @@ do if test f = "$preserve" then touch "$rewritten"/$sha1 - printf '%s\n' "pick $shortsha1 $rest" >> "$todo" + printf '%s\n' "$comment_out $shortsha1 $rest" >> "$todo" fi fi done @@ -851,6 +871,15 @@ cat >> "$todo" << EOF # EOF +if test -z "$keep_empty" +then + cat >> "$todo" << EOF + # Note that commits which are empty at the time of rebasing are + # commented out. +EOF +fi + + has_action "$todo" || die_abort "Nothing to do" diff --git a/git-rebase.sh b/git-rebase.sh index 69c1374..24a2840 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -43,6 +43,7 @@ s,strategy=! use the given merge strategy no-ff! cherry-pick all commits, even if unchanged m,merge! use merging strategies to rebase i,interactive! let the user edit the list of commits to rebase +k,keep-empty preserve empty commits during rebase f,force-rebase! force rebase even if branch is up to date X,strategy-option=! pass the argument through to the merge strategy stat! display a diffstat of what changed upstream @@ -97,6 +98,7 @@ state_dir= action= preserve_merges= autosquash= +keep_empty= test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t read_basic_state () { @@ -220,6 +222,9 @@ do -i) interactive_rebase=explicit ;; + -k) + keep_empty=yes + ;; -p) preserve_merges=t test -z "$interactive_rebase" && interactive_rebase=implied -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] git-rebase: add keep_empty flag 2012-04-11 20:21 ` [PATCH v4 4/4] git-rebase: add keep_empty flag Neil Horman @ 2012-04-11 21:08 ` Junio C Hamano 2012-04-11 23:59 ` Neil Horman 2012-04-12 15:58 ` Neil Horman 0 siblings, 2 replies; 16+ messages in thread From: Junio C Hamano @ 2012-04-11 21:08 UTC (permalink / raw) To: Neil Horman; +Cc: git, Jeff King, Phil Hord, Junio C Hamano This looks quite nice, but it seems that the change breaks at least t3416 and t3404. Has this series been tested? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] git-rebase: add keep_empty flag 2012-04-11 21:08 ` Junio C Hamano @ 2012-04-11 23:59 ` Neil Horman 2012-04-12 3:16 ` Junio C Hamano 2012-04-12 15:58 ` Neil Horman 1 sibling, 1 reply; 16+ messages in thread From: Neil Horman @ 2012-04-11 23:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Phil Hord On Wed, Apr 11, 2012 at 02:08:54PM -0700, Junio C Hamano wrote: > This looks quite nice, but it seems that the change breaks at least t3416 > and t3404. Has this series been tested? > I did not, mostly because those tests didn't seem to work with empty commits at all. And rebases (interactive and automatic) that I've done manually in testing worked fine. I'll check on them in the AM though. Neil ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] git-rebase: add keep_empty flag 2012-04-11 23:59 ` Neil Horman @ 2012-04-12 3:16 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2012-04-12 3:16 UTC (permalink / raw) To: Neil Horman; +Cc: git, Jeff King, Phil Hord Neil Horman <nhorman@tuxdriver.com> writes: > I did not, mostly because those tests didn't seem to work with empty commits at > all. Yeah, existing tests won't even try to rebase an empty or unnecessary commit, so if the change breaks them, you found regressions. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/4] git-rebase: add keep_empty flag 2012-04-11 21:08 ` Junio C Hamano 2012-04-11 23:59 ` Neil Horman @ 2012-04-12 15:58 ` Neil Horman 1 sibling, 0 replies; 16+ messages in thread From: Neil Horman @ 2012-04-12 15:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Phil Hord On Wed, Apr 11, 2012 at 02:08:54PM -0700, Junio C Hamano wrote: > This looks quite nice, but it seems that the change breaks at least t3416 > and t3404. Has this series been tested? > I've looked at these, and I think it appears that this is the result of the changes I made to interactive rebasing in which empty comments were commented out, and additional notes were added to the commit editor template. It seems to be messing with what the fake editor for these tests is expecting. I've got some things I need to do tonight and tomorrow, but I'll fix this up over the weekend. Thanks! Neil ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-04-12 15:59 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <Enhance git-rebases flexibiilty in handling empty commits> 2012-04-11 20:21 ` [PATCH v4 0/4] Enhance git-rebases flexibiilty in handling empty commits Neil Horman 2012-04-11 20:21 ` [PATCH v4 1/4] git-cherry-pick: add allow-empty option Neil Horman 2012-04-11 20:21 ` [PATCH v4 2/4] git-cherry-pick: Add keep-redundant-commits option Neil Horman 2012-04-11 20:47 ` Junio C Hamano 2012-04-11 23:35 ` Neil Horman 2012-04-11 20:57 ` Junio C Hamano 2012-04-11 23:55 ` Neil Horman 2012-04-12 3:15 ` Junio C Hamano 2012-04-12 10:49 ` Neil Horman 2012-04-11 20:21 ` [PATCH v4 3/4] git-cherry-pick: Add test to validate new options Neil Horman 2012-04-11 21:06 ` Junio C Hamano 2012-04-11 20:21 ` [PATCH v4 4/4] git-rebase: add keep_empty flag Neil Horman 2012-04-11 21:08 ` Junio C Hamano 2012-04-11 23:59 ` Neil Horman 2012-04-12 3:16 ` Junio C Hamano 2012-04-12 15:58 ` Neil Horman
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.