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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Alban

Thanks for looking at this series

On 10/05/2020 12:14, Alban Gruin wrote:
> 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?

and delete the one below?

>> +	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:

Long term I'd like to stop writing the author-script file unless we're 
stopping for a conflict resolution or edit so I'm reluctant to add more 
functionality to it at the moment. Also I tend to view read_env_script() 
it as loading the author details from a file rather than setting up the 
environment for the commit.

> ---- 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?

Yes that would fit the style in the rest of the function

Best Wishes

Phillip

> 
> Cheers,
> Alban
>