Re: [PATCH] cherry-pick: add --keep-existing-origin option

On 28 October 2017 at 22:21, Kevin Daudt [off-list ref] wrote:

On Sat, Oct 28, 2017 at 08:04:40PM +0800, Anthony Wong wrote:

quoted

When cherry-picking from a commit whose commit message already
contains the "(cherry picked from commit ...)" line, this option will
not add another one. This is useful when you are cherry-picking from a
bunch of commits, some are cherry-picks and already contains the
upstream hash but some do not. Use with -x.

Signed-off-by: Anthony Wong <redacted>
---
 Documentation/git-cherry-pick.txt |  8 ++++++++
 builtin/revert.c                  |  2 ++
 sequencer.c                       | 14 ++++++++------
 sequencer.h                       |  1 +
 4 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d35d771fc..7a074511f 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt

@@ -71,6 +71,14 @@ OPTIONS
      development branch), adding this information can be
      useful.

+--keep-existing-origin::
+     This option has to be used with -x to take effect. When
+     cherry-picking from a commit whose commit message already
+     contains the "(cherry picked from commit ...)" line, this
+     option will not add another one. This is useful when you are
+     cherry-picking from a bunch of commits, some are cherry-picks
+     and already contains the upstream hash but some do not.
+
 -r::
      It used to be that the command defaulted to do `-x`
      described above, and `-r` was to disable it.  Now the

diff --git a/builtin/revert.c b/builtin/revert.c
index b9d927eb0..a1900cc1d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -122,6 +122,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, "keep-existing-origin", &opts->keep_existing_origin, N_("do not add another hash if one already exists, use with -x")),
                      OPT_END(),
              };
              options = parse_options_concat(options, cp_extra);

@@ -157,6 +158,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
                              "--ff", opts->allow_ff,
                              "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
                              "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
+                             "--keep-existing-origin", opts->keep_existing_origin,
                              NULL);
      }

diff --git a/sequencer.c b/sequencer.c
index f2a10cc4f..c96add16e 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -1050,12 +1050,14 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
                      strbuf_addstr(&msgbuf, p);

              if (opts->record_origin) {
-                     strbuf_complete_line(&msgbuf);
-                     if (!has_conforming_footer(&msgbuf, NULL, 0))
-                             strbuf_addch(&msgbuf, '\n');
-                     strbuf_addstr(&msgbuf, cherry_picked_prefix);
-                     strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
-                     strbuf_addstr(&msgbuf, ")\n");
+                     if (!opts->keep_existing_origin || strstr(msgbuf.buf, cherry_picked_prefix) == NULL) {
+                             strbuf_complete_line(&msgbuf);
+                             if (!has_conforming_footer(&msgbuf, NULL, 0))
+                                     strbuf_addch(&msgbuf, '\n');
+                             strbuf_addstr(&msgbuf, cherry_picked_prefix);
+                             strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
+                             strbuf_addstr(&msgbuf, ")\n");
+                     }
              }
      }

diff --git a/sequencer.h b/sequencer.h
index 6f3d3df82..a907c0947 100644
--- a/sequencer.h
+++ b/sequencer.h

@@ -24,6 +24,7 @@ struct replay_opts {
      int allow_empty;
      int allow_empty_message;
      int keep_redundant_commits;
+     int keep_existing_origin;
      int verbose;

      int mainline;
--

2.14.1

I'm wondering if it isn't better to detect that there is already an
origin present and not add another one.

Or are there situations where you do want multiple cherry-pick origins?

I don't. But because that's how git behaves now, I don't know if
anyone is relying on that behaviour so I am reluctant to change that.

Thanks,
Anthony

Kevin