git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Cc: Johannes.Schindelin@gmx.de, git@vger.kernel.org,
	gitster@pobox.com, martin.agren@gmail.com, newren@gmail.com,
	t.gummerer@gmail.com
Subject: Re: [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date
Date: Tue, 13 Aug 2019 13:09:46 +0100	[thread overview]
Message-ID: <f34a6bf7-8725-774c-5fde-0349bc8565bd@gmail.com> (raw)
In-Reply-To: <7274e753-f75c-ad9a-9c2c-7ba8abc32971@gmail.com>

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

  reply	other threads:[~2019-08-13 12:09 UTC|newest]

Thread overview: 96+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-06 17:36 [GSoC][PATCHl 0/6] rebase -i: support more options Rohit Ashiwal
2019-08-06 17:36 ` [GSoC][PATCHl 1/6] rebase -i: add --ignore-whitespace flag Rohit Ashiwal
2019-08-07  5:38   ` Junio C Hamano
2019-08-07 20:25     ` Rohit Ashiwal
2019-08-08 16:44   ` Phillip Wood
2019-08-12 17:43     ` Rohit Ashiwal
2019-08-06 17:36 ` [GSoC][PATCHl 2/6] sequencer: add NULL checks under read_author_script Rohit Ashiwal
2019-08-06 17:36 ` [GSoC][PATCHl 3/6] rebase -i: support --committer-date-is-author-date Rohit Ashiwal
2019-08-08 11:29   ` Phillip Wood
2019-08-08 16:00     ` Junio C Hamano
2019-08-06 17:36 ` [GSoC][PATCHl 4/6] sequencer: rename amend_author to author_to_rename Rohit Ashiwal
2019-08-08 11:30   ` Phillip Wood
2019-08-06 17:36 ` [GSoC][PATCHl 5/6] rebase -i: support --ignore-date Rohit Ashiwal
2019-08-07 19:41   ` Johannes Schindelin
2019-08-07 20:22     ` Junio C Hamano
2019-08-07 20:33       ` Rohit Ashiwal
2019-08-08 11:42   ` Phillip Wood
2019-08-08 16:53     ` Phillip Wood
2019-08-06 17:36 ` [GSoC][PATCHl 6/6] rebase: add --author-date-is-committer-date Rohit Ashiwal
2019-08-08 11:42   ` Phillip Wood
2019-08-12 19:42 ` [GSoC][PATCH v2 0/6] rebase -i: support more options Rohit Ashiwal
2019-08-12 19:42   ` [GSoC][PATCH v2 1/6] rebase -i: add --ignore-whitespace flag Rohit Ashiwal
2019-08-13 12:07     ` Phillip Wood
2019-08-12 19:42   ` [GSoC][PATCH v2 2/6] sequencer: add NULL checks under read_author_script Rohit Ashiwal
2019-08-12 19:42   ` [GSoC][PATCH v2 3/6] rebase -i: support --committer-date-is-author-date Rohit Ashiwal
2019-08-13 10:38     ` Phillip Wood
2019-08-13 12:09       ` Phillip Wood [this message]
2019-08-13 17:06       ` Junio C Hamano
2019-08-14 18:38         ` Phillip Wood
2019-08-13 13:35     ` Phillip Wood
2019-08-12 19:42   ` [GSoC][PATCH v2 4/6] sequencer: rename amend_author to author_to_rename Rohit Ashiwal
2019-08-13 13:29     ` Phillip Wood
2019-08-12 19:42   ` [GSoC][PATCH v2 5/6] rebase -i: support --ignore-date Rohit Ashiwal
2019-08-13 13:28     ` Phillip Wood
2019-08-13 17:21       ` Junio C Hamano
2019-08-14 18:47         ` Phillip Wood
2019-08-13 21:45     ` Junio C Hamano
2019-08-14 18:51       ` Phillip Wood
2019-08-14 19:33         ` Junio C Hamano
2019-08-17  9:28           ` Phillip Wood
2019-08-12 19:43   ` [GSoC][PATCH v2 6/6] rebase: add --author-date-is-committer-date Rohit Ashiwal
2019-08-13 17:28     ` Junio C Hamano
2019-08-20  3:45 ` [PATCH v3 0/6] rebase -i: support more options Rohit Ashiwal
2019-08-20  3:45   ` [PATCH v3 1/6] rebase -i: add --ignore-whitespace flag Rohit Ashiwal
2019-08-20 18:40     ` Phillip Wood
2019-08-20 18:47       ` Rohit Ashiwal
2019-08-20  3:45   ` [PATCH v3 2/6] sequencer: add NULL checks under read_author_script Rohit Ashiwal
2019-08-23 15:20     ` Junio C Hamano
2019-08-20  3:45   ` [PATCH v3 3/6] rebase -i: support --committer-date-is-author-date Rohit Ashiwal
2019-08-20 13:32     ` Phillip Wood
2019-08-20  3:45   ` [PATCH v3 4/6] sequencer: rename amend_author to author_to_rename Rohit Ashiwal
2019-08-20  3:45   ` [PATCH v3 5/6] rebase -i: support --ignore-date Rohit Ashiwal
2019-08-20 13:45     ` Phillip Wood
2019-08-20 17:42     ` Junio C Hamano
2019-08-20 18:30       ` Phillip Wood
2019-08-20  3:45   ` [GSoC][PATCH v2 6/6] rebase: add --author-date-is-committer-date Rohit Ashiwal
2019-08-20  4:00     ` Rohit Ashiwal
2019-08-20  3:45   ` [PATCH v3 6/6] rebase: add --reset-author-date Rohit Ashiwal
2019-08-20  3:54   ` [PATCH v3 0/6] rebase -i: support more options Rohit Ashiwal
2019-08-20 13:56   ` Phillip Wood
2019-08-20 17:53     ` Junio C Hamano
2019-08-20 18:37       ` Phillip Wood
2019-09-07 11:50 ` [PATCH v4 " Rohit Ashiwal
2019-09-07 11:50   ` [PATCH v4 1/6] rebase -i: add --ignore-whitespace flag Rohit Ashiwal
2019-10-04  9:29     ` Phillip Wood
2019-10-05 18:12       ` Elijah Newren
2019-10-06 17:57       ` Rohit Ashiwal
2019-09-07 11:50   ` [PATCH v4 2/6] sequencer: allow callers of read_author_script() to ignore fields Rohit Ashiwal
2019-09-07 11:50   ` [PATCH v4 3/6] rebase -i: support --committer-date-is-author-date Rohit Ashiwal
2019-10-04  9:37     ` Phillip Wood
2019-10-06 17:57       ` Rohit Ashiwal
2019-10-24 13:28     ` Phillip Wood
2019-09-07 11:50   ` [PATCH v4 4/6] sequencer: rename amend_author to author_to_rename Rohit Ashiwal
2019-09-07 11:50   ` [PATCH v4 5/6] rebase -i: support --ignore-date Rohit Ashiwal
2019-09-07 20:56     ` Rohit Ashiwal
2019-09-27 10:00     ` Phillip Wood
2019-10-06 17:57       ` Rohit Ashiwal
2019-10-24 20:36         ` Phillip Wood
2019-09-07 11:50   ` [PATCH v4 6/6] rebase: add --reset-author-date Rohit Ashiwal
2019-09-09 18:02   ` [PATCH v4 0/6] rebase -i: support more options Junio C Hamano
2019-09-09 18:51     ` Phillip Wood
2019-09-09 19:24       ` Junio C Hamano
2019-11-01 13:59 ` [PATCH v5 " Rohit Ashiwal
2019-11-01 13:59   ` [PATCH v5 1/6] rebase -i: add --ignore-whitespace flag Rohit Ashiwal
2019-11-01 13:59   ` [PATCH v5 2/6] sequencer: allow callers of read_author_script() to ignore fields Rohit Ashiwal
2019-11-01 14:00   ` [PATCH v5 3/6] rebase -i: support --committer-date-is-author-date Rohit Ashiwal
2019-11-01 14:00   ` [PATCH v5 4/6] sequencer: rename amend_author to author_to_rename Rohit Ashiwal
2019-11-01 14:00   ` [PATCH v5 5/6] rebase -i: support --ignore-date Rohit Ashiwal
2019-11-02  7:32     ` Junio C Hamano
2019-11-02  7:48       ` Junio C Hamano
2019-11-01 14:00   ` [PATCH v5 6/6] rebase: add --reset-author-date Rohit Ashiwal
2019-11-02  7:34     ` Junio C Hamano
2019-11-21  6:14   ` [PATCH v5 0/6] rebase -i: support more options Junio C Hamano
2019-11-21  8:17     ` Alban Gruin
2019-11-22  6:32       ` Junio C Hamano
2019-11-28 11:14   ` Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f34a6bf7-8725-774c-5fde-0349bc8565bd@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=martin.agren@gmail.com \
    --cc=newren@gmail.com \
    --cc=rohit.ashiwal265@gmail.com \
    --cc=t.gummerer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).