Neil Horman [off-list ref] writes:
quoted hunk
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.
quoted hunk
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?
On Wed, Apr 11, 2012 at 01:57:24PM -0700, Junio C Hamano wrote:
Neil Horman [off-list ref] writes:
quoted
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.
quoted
+ 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.
quoted
+--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
quoted
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.
quoted
+ 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()
quoted
+
+ 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.
quoted
+ 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?
quoted
+ 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