Re: [PATCH v6 09/10] replace: check mergetags when using --graft
From: Junio C Hamano > > Christian Couder writes: > >> When using --graft, with a mergetag in the original >> commit, we should check that the commit pointed to by >> the mergetag is still a parent of then new commit we >> create, otherwise the mergetag could be misleading. >> >> If the commit pointed to by the mergetag is no more >> a parent of the new commit, we could remove the >> mergetag, but in this case there is a good chance >> that the title or other elements of the commit might >> also be misleading. So let's just error out and >> suggest to use --edit instead on the commit. > > I do not quite understand the reasoning. If you have a merge you > earlier made with a signed tag, and then want to redo the merge with > an updated tip of that branch (perhaps the side branch was earlier > based on maint but now was rebased on master), it will perfectly be > normal to expect that the title or other elements of the resulting > merge to stay the same. Yeah, but then you might also want to have a mergetag for the updated tip of the branch and --graft will not put it in the new commit, so it would be better to use --edit in this case. > Why is it a good idea to error it out? Because sometimes, in complex cases, it is misleading to do as if you can do the right thing, when there is a good chance you cannot. > If the argument were '"replace --graft" that changes the parents is > likely to affect merge summary message, so error out and suggest to > use --edit instead', regardless of the 'mergetag', I'd understand > it, but that would essentially mean that 'replace --graft' should > never be used, so... I think that when "replace --graft" is used on a regular commit there is much better chance that the resulting replacement commit will be as the user expect it to be. Thanks, Christian. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 09/10] replace: check mergetags when using --graft
Christian Couder writes: > When using --graft, with a mergetag in the original > commit, we should check that the commit pointed to by > the mergetag is still a parent of then new commit we > create, otherwise the mergetag could be misleading. > > If the commit pointed to by the mergetag is no more > a parent of the new commit, we could remove the > mergetag, but in this case there is a good chance > that the title or other elements of the commit might > also be misleading. So let's just error out and > suggest to use --edit instead on the commit. I do not quite understand the reasoning. If you have a merge you earlier made with a signed tag, and then want to redo the merge with an updated tip of that branch (perhaps the side branch was earlier based on maint but now was rebased on master), it will perfectly be normal to expect that the title or other elements of the resulting merge to stay the same. Why is it a good idea to error it out? If the argument were '"replace --graft" that changes the parents is likely to affect merge summary message, so error out and suggest to use --edit instead', regardless of the 'mergetag', I'd understand it, but that would essentially mean that 'replace --graft' should never be used, so... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 09/10] replace: check mergetags when using --graft
When using --graft, with a mergetag in the original
commit, we should check that the commit pointed to by
the mergetag is still a parent of then new commit we
create, otherwise the mergetag could be misleading.
If the commit pointed to by the mergetag is no more
a parent of the new commit, we could remove the
mergetag, but in this case there is a good chance
that the title or other elements of the commit might
also be misleading. So let's just error out and
suggest to use --edit instead on the commit.
Signed-off-by: Christian Couder
---
builtin/replace.c | 47 +++
1 file changed, 47 insertions(+)
diff --git a/builtin/replace.c b/builtin/replace.c
index cc29ef2..2290529 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -13,6 +13,7 @@
#include "refs.h"
#include "parse-options.h"
#include "run-command.h"
+#include "tag.h"
static const char * const git_replace_usage[] = {
N_("git replace [-f] "),
@@ -325,6 +326,50 @@ static void replace_parents(struct strbuf *buf, int argc,
const char **argv)
strbuf_release(&new_parents);
}
+struct check_mergetag_data {
+ int argc;
+ const char **argv;
+};
+
+static void check_one_mergetag(struct commit *commit,
+ struct commit_extra_header *extra,
+ void *data)
+{
+ struct check_mergetag_data *mergetag_data = (struct check_mergetag_data
*)data;
+ const char *ref = mergetag_data->argv[0];
+ unsigned char tag_sha1[20];
+ struct tag *tag;
+ int i;
+
+ hash_sha1_file(extra->value, extra->len, typename(OBJ_TAG), tag_sha1);
+ tag = lookup_tag(tag_sha1);
+ if (!tag)
+ die(_("bad mergetag in commit '%s'"), ref);
+ if (parse_tag_buffer(tag, extra->value, extra->len))
+ die(_("malformed mergetag in commit '%s'"), ref);
+
+ /* iterate over new parents */
+ for (i = 1; i < mergetag_data->argc; i++) {
+ unsigned char sha1[20];
+ if (get_sha1(mergetag_data->argv[i], sha1) < 0)
+ die(_("Not a valid object name: '%s'"),
mergetag_data->argv[i]);
+ if (!hashcmp(tag->tagged->sha1, sha1))
+ return; /* found */
+ }
+
+ die(_("original commit '%s' contains mergetag '%s' that is discarded; "
+ "use --edit instead of --graft"), ref, sha1_to_hex(tag_sha1));
+}
+
+static void check_mergetags(struct commit *commit, int argc, const char **argv)
+{
+ struct check_mergetag_data mergetag_data;
+
+ mergetag_data.argc = argc;
+ mergetag_data.argv = argv;
+ for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
+}
+
static int create_graft(int argc, const char **argv, int force)
{
unsigned char old[20], new[20];
@@ -349,6 +394,8 @@ static int create_graft(int argc, const char **argv, int
force)
warning(_("the signature will be removed in the replacement
commit!"));
}
+ check_mergetags(commit, argc, argv);
+
if (write_sha1_file(buf.buf, buf.len, commit_type, new))
die(_("could not write replacement commit for: '%s'"), old_ref);
--
2.0.0.421.g786a89d.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
