[v4,0/6] rebase -i: support more options
mbox series

Message ID 20190907115034.14933-1-rohit.ashiwal265@gmail.com
Headers show
Series
  • rebase -i: support more options
Related show

Message

Rohit Ashiwal Sept. 7, 2019, 11:50 a.m. UTC
Following the suggestion of Phillip I've rebased my patch on master (745f681289)
and cherry-picking b0a3186140.

Base: 745f6812895b31c02b29bdfe4ae8e5498f776c26
      with cherry-picked b0a3186140dbc7bd64cbc6ef733386a0f1eb6a4d

Outline:
  2/6:
      - Change commit message and description
  3/6:
      - add test for rebase -r
      - add strict bounds while pushing ident date
  5/6:
      - bug fix: (following am's code) if --ignore-date is provided,
                 we should setenv GIT_COMMITTER_DATE to "".
      - catch error when ident line is malformed
      - push GIT_AUTHOR_DATE instead of providing it with --date so
        that "git merge" can also use push_dates function
      - add test for rebase -r


Rohit Ashiwal (6):
  rebase -i: add --ignore-whitespace flag
  sequencer: allow callers of read_author_script() to ignore fields
  rebase -i: support --committer-date-is-author-date
  sequencer: rename amend_author to author_to_rename
  rebase -i: support --ignore-date
  rebase: add --reset-author-date

 Documentation/git-rebase.txt            |  26 +++--
 builtin/rebase.c                        |  55 +++++++---
 sequencer.c                             | 134 +++++++++++++++++++++--
 sequencer.h                             |   2 +
 t/t3422-rebase-incompatible-options.sh  |   2 -
 t/t3433-rebase-options-compatibility.sh | 137 ++++++++++++++++++++++++
 6 files changed, 328 insertions(+), 28 deletions(-)
 create mode 100755 t/t3433-rebase-options-compatibility.sh

Range-diff:
--- snip ---
 1:  e82ed8cad5 = 49:  77af1d66db rebase -i: add --ignore-whitespace flag
 2:  209057b361 ! 50:  1f7f1407b2 sequencer: add NULL checks under read_author_script
    @@ -1,19 +1,14 @@
     Author: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
     
    -    sequencer: add NULL checks under read_author_script
    +    sequencer: allow callers of read_author_script() to ignore fields
     
    -    read_author_script reads name, email and author date from the author
    -    script. However, it does not check if the arguments are NULL. Adding
    -    NULL checks will allow us to selectively get the required value, for
    -    example:
    +    The current callers of the read_author_script() function read name,
    +    email and date from the author script.  Allow callers to signal that
    +    they are not interested in some among these three fields by passing
    +    NULL.
     
    -        char *date;
    -        if (read_author_script(_path_, NULL, NULL, &date, _int_))
    -                die(_("failed to read author date"));
    -        /* needs to be free()'d */
    -        return date;
    -
    -    Add NULL checks for better control over the information retrieved.
    +    Note that fields that are ignored still must exist and be formatted
    +    correctly in the author script.
     
         Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
     
 3:  a4e6644ef8 ! 51:  cc1614154e rebase -i: support --committer-date-is-author-date
    @@ -128,35 +128,34 @@
     +	return date;
     +}
     +
    - /* Read author-script and return an ident line (author <email> timestamp) */
    - static const char *read_author_ident(struct strbuf *buf)
    - {
    + static const char staged_changes_advice[] =
    + N_("you have staged changes in your working tree\n"
    + "If these changes are meant to be squashed into the previous commit, run:\n"
     @@
    - {
    - 	struct child_process cmd = CHILD_PROCESS_INIT;
      
    -+	if (opts->committer_date_is_author_date) {
    -+		size_t len;
    -+		int res = -1;
    -+		struct strbuf datebuf = STRBUF_INIT;
    -+		char *date = read_author_date_or_null();
    + 	cmd.git_cmd = 1;
    + 
    ++        if (opts->committer_date_is_author_date) {
    ++	        int res = -1;
    ++	        struct strbuf datebuf = STRBUF_INIT;
    ++	        char *date = read_author_date_or_null();
     +
    -+		if (!date)
    -+			return -1;
    ++	        if (!date)
    ++	                return -1;
     +
    -+		strbuf_addf(&datebuf, "@%s", date);
    -+		free(date);
    ++	        strbuf_addf(&datebuf, "@%s", date);
    ++	        res = setenv("GIT_COMMITTER_DATE", datebuf.buf, 1);
     +
    -+		date = strbuf_detach(&datebuf, &len);
    -+		res = setenv("GIT_COMMITTER_DATE", date, 1);
    -+		free(date);
    ++		strbuf_release(&datebuf);
    ++	        free(date);
     +
    -+		if (res)
    -+			return -1;
    -+	}
    - 	if ((flags & CREATE_ROOT_COMMIT) && !(flags & AMEND_MSG)) {
    - 		struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
    - 		const char *author = NULL;
    ++	        if (res)
    ++	                return -1;
    ++        }
    ++
    + 	if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
    + 		const char *gpg_opt = gpg_sign_opt_quoted(opts);
    + 
     @@
      
      	if (parse_head(r, &current_head))
    @@ -174,14 +173,19 @@
     +		struct ident_split ident;
     +		struct strbuf date = STRBUF_INIT;
     +
    -+		split_ident_line(&ident, author, len);
    -+
    ++		if (split_ident_line(&ident, author, len) < 0)
    ++			return error(_("malformed ident line"));
     +		if (!ident.date_begin)
     +			return error(_("corrupted author without date information"));
     +
    -+		strbuf_addf(&date, "@%s", ident.date_begin);
    -+		setenv("GIT_COMMITTER_DATE", date.buf, 1);
    ++		strbuf_addf(&date, "@%.*s %.*s",
    ++			    (int)(ident.date_end - ident.date_begin), ident.date_begin,
    ++			    (int)(ident.tz_end - ident.tz_begin), ident.tz_begin);
    ++		res = setenv("GIT_COMMITTER_DATE", date.buf, 1);
     +		strbuf_release(&date);
    ++
    ++		if (res)
    ++			goto out;
     +	}
     +
      	if (write_index_as_tree(&tree, r->index, r->index_file, 0, NULL)) {
    @@ -256,6 +260,20 @@
      # This is a special case in which both am and interactive backends
      # provide the same output. It was done intentionally because
      # both the backends fall short of optimal behaviour.
    +@@
    + 	EOF
    + 	git commit -am "update file" &&
    + 	git tag side &&
    ++	test_commit commit1 foo foo1 &&
    ++	test_commit commit2 foo foo2 &&
    ++	test_commit commit3 foo foo3 &&
    + 
    + 	git checkout --orphan master &&
    ++	git rm --cached foo &&
    ++	rm foo &&
    + 	sed -e "s/^|//" >file <<-\EOF &&
    + 	|line 1
    + 	|        line 2
     @@
      	test_cmp expect file
      '
    @@ -275,5 +293,18 @@
     +	git show HEAD --pretty="format:%ci" >committertime &&
     +	test_cmp authortime committertime
     +'
    ++
    ++test_expect_success '--committer-date-is-author-date works with rebase -r' '
    ++	git checkout side &&
    ++	git merge commit3 &&
    ++	git rebase -r --root --committer-date-is-author-date &&
    ++	git rev-list HEAD >rev_list &&
    ++	while read HASH
    ++	do
    ++		git show $HASH --pretty="format:%ai" >authortime
    ++		git show $HASH --pretty="format:%ci" >committertime
    ++		test_cmp authortime committertime
    ++	done <rev_list
    ++'
     +
      test_done
 4:  6ac1885c54 = 52:  9e92c79bda sequencer: rename amend_author to author_to_rename
 5:  a69749dd67 ! 53:  fc68e55e78 rebase -i: support --ignore-date
    @@ -83,8 +83,10 @@
      	}
      
     -	if (options.committer_date_is_author_date)
    -+	if (options.ignore_date)
    ++	if (options.ignore_date) {
     +		options.committer_date_is_author_date = 0;
    ++		setenv("GIT_COMMITTER_DATE", "", 1);
    ++	}
     +	if (options.committer_date_is_author_date ||
     +	    options.ignore_date)
      		options.flags |= REBASE_FORCE;
    @@ -110,7 +112,7 @@
      static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
      static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
     @@
    - 	return buf->buf;
    + 	return date;
      }
      
     +/* Construct a free()able author string with current time as the author date */
    @@ -120,7 +122,10 @@
     +	struct ident_split ident;
     +	struct strbuf new_author = STRBUF_INIT;
     +
    -+	split_ident_line(&ident, author, len);
    ++	if (split_ident_line(&ident, author, len) < 0) {
    ++		error(_("malformed ident line"));
    ++		return NULL;
    ++	}
     +	len = ident.mail_end - ident.name_begin + 1;
     +
     +	strbuf_addf(&new_author, "%.*s ", len, author);
    @@ -134,7 +139,7 @@
     +	struct strbuf date = STRBUF_INIT;
     +
     +	strbuf_addf(&date, "@%"PRIuMAX, (uintmax_t)now);
    -+	argv_array_pushf(&child->args, "--date=%s", date.buf);
    ++	argv_array_pushf(&child->env_array, "GIT_AUTHOR_DATE=%s", date.buf);
     +	argv_array_pushf(&child->env_array, "GIT_COMMITTER_DATE=%s", date.buf);
     +	strbuf_release(&date);
     +}
    @@ -143,28 +148,6 @@
      N_("you have staged changes in your working tree\n"
      "If these changes are meant to be squashed into the previous commit, run:\n"
     @@
    - 
    - 		if (res <= 0)
    - 			res = error_errno(_("could not read '%s'"), defmsg);
    --		else
    -+		else {
    -+			if (opts->ignore_date) {
    -+				char *new_author = ignore_author_date(author);
    -+				if (!author)
    -+					BUG("ignore-date can only be used with "
    -+					    "rebase, which must set the author "
    -+					    "before committing the tree");
    -+				free((void *)author);
    -+				author = new_author;
    -+			}
    - 			res = commit_tree(msg.buf, msg.len, cache_tree_oid,
    - 					  NULL, &root_commit, author,
    - 					  opts->gpg_sign);
    -+		}
    - 
    - 		strbuf_release(&msg);
    - 		strbuf_release(&script);
    -@@
      		argv_array_push(&cmd.args, "--amend");
      	if (opts->gpg_sign)
      		argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
    @@ -179,6 +162,10 @@
      
     +	if (opts->ignore_date) {
     +		author = ignore_author_date(author);
    ++		if (!author) {
    ++			res = -1;
    ++			goto out;
    ++		}
     +		free(author_to_free);
     +		author_to_free = (char *)author;
     +	}
    @@ -242,7 +229,7 @@
      --- a/t/t3433-rebase-options-compatibility.sh
      +++ b/t/t3433-rebase-options-compatibility.sh
     @@
    - 	test_cmp authortime committertime
    + 	done <rev_list
      '
      
     +# Checking for +0000 in author time is enough since default
    @@ -252,13 +239,32 @@
     +	git commit --amend --date="$GIT_AUTHOR_DATE" &&
     +	git rebase --ignore-date HEAD^ &&
     +	git show HEAD --pretty="format:%ai" >authortime &&
    -+	grep "+0000" authortime
    ++	git show HEAD --pretty="format:%ci" >committertime &&
    ++	grep "+0000" authortime &&
    ++	grep "+0000" committertime
     +'
     +
     +test_expect_success '--ignore-date works with interactive backend' '
     +	git commit --amend --date="$GIT_AUTHOR_DATE" &&
     +	git rebase --ignore-date -i HEAD^ &&
     +	git show HEAD --pretty="format:%ai" >authortime &&
    -+	grep "+0000" authortime
    ++	git show HEAD --pretty="format:%ci" >committertime &&
    ++	grep "+0000" authortime &&
    ++	grep "+0000" committertime
     +'
    ++
    ++test_expect_success '--ignore-date works with rebase -r' '
    ++	git checkout side &&
    ++	git merge commit3 &&
    ++	git rebase -r --root --ignore-date &&
    ++	git rev-list HEAD >rev_list &&
    ++	while read HASH
    ++	do
    ++		git show $HASH --pretty="format:%ai" >authortime
    ++		git show $HASH --pretty="format:%ci" >committertime
    ++		grep "+0000" authortime
    ++		grep "+0000" committertime
    ++	done <rev_list
    ++'
    ++
      test_done
 6:  210d15cca0 = 54:  396d5f16eb rebase: add --reset-author-date

Comments

Junio C Hamano Sept. 9, 2019, 6:02 p.m. UTC | #1
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

> Following the suggestion of Phillip I've rebased my patch on master (745f681289)
> and cherry-picking b0a3186140.

Sorry, but that's horrible.  The latter does not even cleanly apply
on the former.

Let me see if I can find time to whip this into a reasonable shape.

Thanks.
Phillip Wood Sept. 9, 2019, 6:51 p.m. UTC | #2
On 09/09/2019 19:02, Junio C Hamano wrote:
> Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:
> 
>> Following the suggestion of Phillip I've rebased my patch on master (745f681289)
>> and cherry-picking b0a3186140.
> 
> Sorry, but that's horrible.  The latter does not even cleanly apply
> on the former.

Yes I had assumed that the cherry pick would become the first patch of 
this series and be dropped from pw/rebase-i-show-HEAD-to-reword. I 
should have been more explicit about that.

> Let me see if I can find time to whip this into a reasonable shape.

As pw/rebase-i-show-HEAD-to-reword is slated for next perhaps these 
could build on that. The first patch needs am -3 to apply to that branch 
but the result looks ok and the rest apply as is.

Best Wishes

Phillip

> Thanks.
>
Junio C Hamano Sept. 9, 2019, 7:24 p.m. UTC | #3
Phillip Wood <phillip.wood123@gmail.com> writes:

> Yes I had assumed that the cherry pick would become the first patch of
> this series and be dropped from pw/rebase-i-show-HEAD-to-reword.

Ah, such an arrangement would have made a log more sense, indeed.

> As pw/rebase-i-show-HEAD-to-reword is slated for next perhaps these
> could build on that. The first patch needs am -3 to apply to that
> branch but the result looks ok and the rest apply as is.

As this is more or less "new feature" series, I do not mind too much
to make it depend on another topic in flight that is getting solid.

Thanks for helping.