Re: [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date

[Phillip Wood <phillip.wood123@xxxxxxxxx>]

On 13/08/2019 11:38, Phillip Wood wrote:
> Hi Rohit
>
> [...]
> > @@ -964,6 +976,25 @@ static int run_git_commit(struct repository *r,
> >   {
> >       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();
>
> You must always check the return value of functions that might return 
> NULL. In this case we should return an error as you do in try_to 
> _commit() later
>
> > +
> > +        strbuf_addf(&datebuf, "@%s", date);
>
> GNU printf() will add something like '(null)' to the buffer if you pass 
> a NULL pointer so I don't think we can be sure that this will not 
> increase the length of the buffer if date is NULL.

I should have added that passing NULL to snprintf() and friends is going 
to be undefined behavior anyway so you shouldn't do it for that reason 
alone.

Best Wishes

Phillip

 An explicit check
> above would be much clearer as well rather than checking len later. What 
> happens if you don't add the '@' at the beginning? (I'm don't know much 
> about git's date handling)
>
> > +        free(date);
> > +
> > +        date = strbuf_detach(&datebuf, &len);
> > +
> > +        if (len > 1)
> > +            res = setenv("GIT_COMMITTER_DATE", date, 1);
> > +
> > +        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;
> > @@ -1400,7 +1431,6 @@ static int try_to_commit(struct repository *r,
> >       if (parse_head(r, &current_head))
> >           return -1;
> > -
> >       if (flags & AMEND_MSG) {
> >           const char *exclude_gpgsig[] = { "gpgsig", NULL };
> >           const char *out_enc = get_commit_output_encoding();
> > @@ -1427,6 +1457,21 @@ static int try_to_commit(struct repository *r,
> >           commit_list_insert(current_head, &parents);
> >       }
> > +    if (opts->committer_date_is_author_date) {
> > +        int len = strlen(author);
> > +        struct ident_split ident;
> > +        struct strbuf date = STRBUF_INIT;
> > +
> > +        split_ident_line(&ident, author, len);
> > +
> > +        if (!ident.date_begin)
> > +            return error(_("corrupted author without date 
> > information"));
>
> We return an error if we cannot get the date - this is exactly what we 
> should be doing above. It is also great to see a single version of this 
> being used whether or not we are amending.
>
> > +
> > +        strbuf_addf(&date, "@%s",ident.date_begin);
>
> I think we should use %s.* and ident.date_end to be sure we getting what 
> we want. Your version is OK if the author is formatted correctly but I'm 
> uneasy about relying on that when we can get the verified end from ident.
>
> Best Wishes
>
> Phillip
>
> > +        setenv("GIT_COMMITTER_DATE", date.buf, 1);
> > +        strbuf_release(&date);
> > +    }
> > +
> >       if (write_index_as_tree(&tree, r->index, r->index_file, 0, NULL)) {
> >           res = error(_("git write-tree failed to write a tree"));
> >           goto out;
> > @@ -2542,6 +2587,11 @@ static int read_populate_opts(struct 
> > replay_opts *opts)
> >               opts->signoff = 1;
> >           }
> > +        if (file_exists(rebase_path_cdate_is_adate())) {
> > +            opts->allow_ff = 0;
> > +            opts->committer_date_is_author_date = 1;
> > +        }
> > +
> >           if (file_exists(rebase_path_reschedule_failed_exec()))
> >               opts->reschedule_failed_exec = 1;
> > @@ -2624,6 +2674,8 @@ int write_basic_state(struct replay_opts *opts, 
> > const char *head_name,
> >           write_file(rebase_path_gpg_sign_opt(), "-S%s\n", 
> > opts->gpg_sign);
> >       if (opts->signoff)
> >           write_file(rebase_path_signoff(), "--signoff\n");
> > +    if (opts->committer_date_is_author_date)
> > +        write_file(rebase_path_cdate_is_adate(), "%s", "");
> >       if (opts->reschedule_failed_exec)
> >           write_file(rebase_path_reschedule_failed_exec(), "%s", "");
> > @@ -3821,7 +3873,8 @@ static int pick_commits(struct repository *r,
> >       setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> >       if (opts->allow_ff)
> >           assert(!(opts->signoff || opts->no_commit ||
> > -                opts->record_origin || opts->edit));
> > +                opts->record_origin || opts->edit ||
> > +                opts->committer_date_is_author_date));
> >       if (read_and_refresh_cache(r, opts))
> >           return -1;
> > diff --git a/sequencer.h b/sequencer.h
> > index 6704acbb9c..e3881e9275 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -43,6 +43,7 @@ struct replay_opts {
> >       int verbose;
> >       int quiet;
> >       int reschedule_failed_exec;
> > +    int committer_date_is_author_date;
> >       int mainline;
> > diff --git a/t/t3422-rebase-incompatible-options.sh 
> > b/t/t3422-rebase-incompatible-options.sh
> > index 4342f79eea..7402f7e3da 100755
> > --- a/t/t3422-rebase-incompatible-options.sh
> > +++ b/t/t3422-rebase-incompatible-options.sh
> > @@ -61,7 +61,6 @@ test_rebase_am_only () {
> >   }
> >   test_rebase_am_only --whitespace=fix
> > -test_rebase_am_only --committer-date-is-author-date
> >   test_rebase_am_only -C4
> >   test_expect_success REBASE_P '--preserve-merges incompatible with 
> > --signoff' '
> > diff --git a/t/t3433-rebase-options-compatibility.sh 
> > b/t/t3433-rebase-options-compatibility.sh
> > index 2e16e00a9d..b2419a2b75 100755
> > --- a/t/t3433-rebase-options-compatibility.sh
> > +++ b/t/t3433-rebase-options-compatibility.sh
> > @@ -7,6 +7,9 @@ test_description='tests to ensure compatibility 
> > between am and interactive backe
> >   . ./test-lib.sh
> > +GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30"
> > +export GIT_AUTHOR_DATE
> > +
> >   # 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.
> > @@ -62,4 +65,20 @@ test_expect_success '--ignore-whitespace works with 
> > interactive backend' '
> >       test_cmp expect file
> >   '
> > +test_expect_success '--committer-date-is-author-date works with am 
> > backend' '
> > +    git commit --amend &&
> > +    git rebase --committer-date-is-author-date HEAD^ &&
> > +    git show HEAD --pretty="format:%ai" >authortime &&
> > +    git show HEAD --pretty="format:%ci" >committertime &&
> > +    test_cmp authortime committertime
> > +'
> > +
> > +test_expect_success '--committer-date-is-author-date works with 
> > interactive backend' '
> > +    git commit --amend &&
> > +    git rebase -i --committer-date-is-author-date HEAD^ &&
> > +    git show HEAD --pretty="format:%ai" >authortime &&
> > +    git show HEAD --pretty="format:%ci" >committertime &&
> > +    test_cmp authortime committertime
> > +'
> > +
> >   test_done