Re: [PATCH 1/2] commit: add message options for rebase --autosquash

On Fri, Sep 17, 2010 at 2:36 AM, Stephen Boyd [off-list ref] wrote:

On 09/16/2010 06:39 PM, Pat Notz wrote:

quoted

These options make it convenient to construct commit messages for use
with 'rebase --autosquash'.  The resulting commit message will be
"fixup! ..." or "squash! ..." where "..." is the subject line of the
specified commit message.

Example usage:
  $ git commit --fixup HEAD~2
  $ git commit --squash HEAD~5

Signed-off-by: Pat Notz <redacted>
---

So far I've been using an alias for these, but I suppose making them
real features of git could be worthwhile. What are the benefits with
this approach vs. an alias?

Mainly it's convenience.  The rebase --autosquash feature seems too
hard to use without this or an alias and making everyone code their
own alias seems a lot to ask.

Still, I admit that I was concerned with adding yet another option to
git-commit.  If enough people object, I can live with that.

quoted

@@ -863,7 +871,7 @@ static int parse_and_validate_options(int argc, const char *argv[],

      if (force_author && renew_authorship)
              die("Using both --reset-author and --author does not make sense");

-     if (logfile || message.len || use_message)
+     if (logfile || message.len || use_message || fixup_message || squash_message)
              use_editor = 0;
      if (edit_flag)
              use_editor = 1;

The whole point of squash is to combine two commit texts, right?
Otherwise wouldn't you use --fixup where you throw away the text
eventually and thus don't want to open an editor?

Good point.  Admittedly, I was focusing on the 'fixup' case but squash
needs to open the editor with the first line pre-filled.

quoted

@@ -883,15 +891,19 @@ static int parse_and_validate_options(int argc, const char *argv[],

              f++;
      if (edit_message)
              f++;
+     if (fixup_message)
+             f++;
+     if (squash_message)
+             f++;
      if (logfile)
              f++;
      if (f > 1)
-             die("Only one of -c/-C/-F can be used.");
+             die("Only one of -c/-C/-F/--fixup/--squash can be used.");
      if (message.len && f > 0)
-             die("Option -m cannot be combined with -c/-C/-F.");
+             die("Option -m cannot be combined with -c/-C/-F/--fixup/--squash.");

Furthering that point, perhaps I want to squash this commit into another
commit using the commit text from yet another commit or just with an
extra note from the command line (-m). Perhaps this is where the benefit
over an alias comes in?

That's a good use-case.  I'll re-work the --squash option.

quoted

      if (edit_message)
              use_message = edit_message;
-     if (amend && !use_message)
+     if (amend && (!use_message && !fixup_message && !squash_message))
              use_message = "HEAD";
      if (!use_message && renew_authorship)
              die("--reset-author can be used only with -C, -c or --amend.");

@@ -932,6 +944,23 @@ static int parse_and_validate_options(int argc, const char *argv[],

              if (enc != utf8)
                      free(enc);
      }
+     if (fixup_message || squash_message) {
+             unsigned char sha1[20];
+             struct commit *commit;
+             const char * target_message = fixup_message ? fixup_message : squash_message;
+             const char * msg_fmt = fixup_message ? "fixup! %s" : "squash! %s";

Style nit: stick the * to the variable.

Oops, thanks.

I read this and became confused. fixup_message? target_message? Perhaps
it should be renamed to fixup_commit, squash_commit, target_commit?

I was mostly trying to reduce duplicate code for the two cases... but,
I bet when I re-work --squash this will go away.

quoted

+             struct strbuf buf = STRBUF_INIT;
+             struct pretty_print_context ctx = {0};
+
+             if (get_sha1(target_message, sha1))
+                     die("could not lookup commit %s", target_message);
+             commit = lookup_commit_reference(sha1);
+             if (!commit || parse_commit(commit))
+                     die("could not parse commit %s", target_message);
+
+             format_commit_message(commit, msg_fmt, &buf, &ctx);
+             fixup_message_buffer = strbuf_detach(&buf, NULL);
+     }

Is it necessary to do this block of code here? Couldn't you lookup and
format the commit in prepare_to_commit()? Then we wouldn't have to
allocate another strbuf and the "message" code would be more centralized.

Probably not, I was mostly trying to follow the example from the
use_message (-C/-c) feature.  It *would* be nice to avoid the extra
memory (de)alloc.

Thanks for the great feedback!