Re: [PATCH v2 4/5] rebase -i: support --ignore-date

[Alban Gruin <alban.gruin@gmail.com>]

Hi Phillip,

Le 29/04/2020 à 12:25, Phillip Wood a écrit :
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> As part of the on-going effort to retire the apply rebase backend
> teach the merge backend how to handle --ignore-date. We take care to
> handle the combination of --ignore-date and
> --committer-date-is-author-date in the same way as the apply backend.
> 
> Original-patch-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/git-rebase.txt   |  7 +++--
>  builtin/rebase.c               | 13 +++++---
>  sequencer.c                    | 49 +++++++++++++++++++++++++++--
>  sequencer.h                    |  1 +
>  t/t3436-rebase-more-options.sh | 57 ++++++++++++++++++++++++++++++++++
>  5 files changed, 117 insertions(+), 10 deletions(-)
> 
> -%<-
>
> diff --git a/sequencer.c b/sequencer.c
> index 8dff8b9b95..339dbf4a59 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -150,6 +150,7 @@ static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
>   */
>  static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
>  static GIT_PATH_FUNC(rebase_path_cdate_is_adate, "rebase-merge/cdate_is_adate")
> +static GIT_PATH_FUNC(rebase_path_ignore_date, "rebase-merge/ignore_date")
>  static GIT_PATH_FUNC(rebase_path_orig_head, "rebase-merge/orig-head")
>  static GIT_PATH_FUNC(rebase_path_verbose, "rebase-merge/verbose")
>  static GIT_PATH_FUNC(rebase_path_quiet, "rebase-merge/quiet")
> @@ -889,6 +890,24 @@ static const char *author_date_from_env_array(const struct argv_array *env)
>  	BUG("GIT_AUTHOR_DATE missing from author script");
>  }
>  
> +/* Construct a free()able author string with current time as the author date */
> +static char *ignore_author_date(const char *author)
> +{
> +	int len = strlen(author);
> +	struct ident_split ident;
> +	struct strbuf new_author = STRBUF_INIT;
> +
> +	if (split_ident_line(&ident, author, len) < 0) {
> +		error(_("malformed ident line '%s'"), author);
> +		return NULL;
> +	}

Nit: add an empty line here?

> +	len = ident.mail_end - ident.name_begin + 1;
> +
> +	strbuf_addf(&new_author, "%.*s ", len, ident.name_begin);
> +	datestamp(&new_author);
> +	return strbuf_detach(&new_author, NULL);
> +}
> +
>  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"
> @@ -957,7 +976,11 @@ static int run_git_commit(struct repository *r,
>  
>  	if (opts->committer_date_is_author_date)
>  		argv_array_pushf(&cmd.env_array, "GIT_COMMITTER_DATE=%s",
> +				 opts->ignore_date ?
> +				 "" :
>  				 author_date_from_env_array(&cmd.env_array));
> +	if (opts->ignore_date)
> +		argv_array_push(&cmd.env_array, "GIT_AUTHOR_DATE=");
>  

Perhaps this could be done in read_env_script(), too, instead of fixing
up what this function did right after calling it, twice?  Something like
this:

---- snip ----
index 339dbf4a59..3a7a80bab7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -840,11 +840,12 @@ int read_author_script(const char *path, char
**name, char **email, char **date,
 }

 /*
- * Read a GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL AND GIT_AUTHOR_DATE from a
- * file with shell quoting into struct argv_array. Returns -1 on
- * error, 0 otherwise.
+ * Read a GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE from a
+ * file with shell quoting into struct argv_array, and set
+ * GIT_COMMITTER_DATE if needed. Returns -1 on error, 0 otherwise.
  */
-static int read_env_script(struct argv_array *env)
+static int read_env_script(struct replay_opts *opts,
+                          struct argv_array *env)
 {
        char *name, *email, *date;

@@ -854,7 +855,17 @@ static int read_env_script(struct argv_array *env)

        argv_array_pushf(env, "GIT_AUTHOR_NAME=%s", name);
        argv_array_pushf(env, "GIT_AUTHOR_EMAIL=%s", email);
-       argv_array_pushf(env, "GIT_AUTHOR_DATE=%s", date);
+
+       if (opts->ignore_date) {
+               argv_array_push(env, "GIT_AUTHOR_DATE=");
+               argv_array_push(env, "GIT_COMMITTER_DATE=");
+       } else {
+               argv_array_pushf(env, "GIT_AUTHOR_DATE=%s", date);
+
+               if (opts->committer_date_is_author_date)
+                       argv_array_pushf(env, "GIT_COMMITTER_DATE=%s",
date);
+       }
+
        free(name);
        free(email);
        free(date);
---- snap ----

>  	argv_array_push(&cmd.args, "commit");
>  
> @@ -1386,7 +1409,8 @@ static int try_to_commit(struct repository *r,
>  		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);
> +		res = setenv("GIT_COMMITTER_DATE",
> +			     opts->ignore_date ? "" : date.buf, 1);
>  		strbuf_release(&date);
>  
>  		if (res)
> @@ -1452,6 +1476,15 @@ static int try_to_commit(struct repository *r,
>  
>  	reset_ident_date();
>  
> +	if (opts->ignore_date) {
> +		author = ignore_author_date(author);
> +		if (!author) {
> +			res = -1;
> +			goto out;
> +		}
> +		free(author_to_free);
> +		author_to_free = (char *)author;
> +	}

Nit: add an empty line here?

Cheers,
Alban