git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Rohit Ashiwal <rohit.ashiwal265@gmail.com>,
	Johannes.Schindelin@gmx.de, git@vger.kernel.org,
	martin.agren@gmail.com, newren@gmail.com, t.gummerer@gmail.com
Subject: Re: [GSoC][PATCH v2 5/6] rebase -i: support --ignore-date
Date: Wed, 14 Aug 2019 19:47:54 +0100	[thread overview]
Message-ID: <812513a6-bd83-5830-055f-7c0252e5ecd8@gmail.com> (raw)
In-Reply-To: <xmqqo90t7zhl.fsf@gitster-ct.c.googlers.com>

Hi Junio & Rohit

On 13/08/2019 18:21, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> +static void push_dates(struct child_process *child)
>>> +{
>>> +	time_t now = time(NULL);
>>> +	struct strbuf date = STRBUF_INIT;
>>> +
>>> +	strbuf_addf(&date, "@%"PRIuMAX, (uintmax_t)now);
>>> +	argv_array_pushf(&child->args, "--date=%s", date.buf);
>>
>> it doesn't matter but it might have been nicer to set both dates the
>> same way in the environment.
>> +	argv_array_pushf(&child->env_array, "GIT_COMMITTER_DATE=%s", date.buf);
> 
> We can see that this date string lacks timezone information, which
> would likely fall back to whatever timezone the user is in.  Is that
> what we want?  I am guessing it is, as we are dealing with "now"
> timestamp, but wanted to double check.

I think we probably want to use the local timezone as you suggest

>>> +			if (opts->ignore_date) {
>>> +				if (!author)
>>> +					BUG("ignore-date can only be used with "
>>> +					    "rebase, which must set the author "
>>> +					    "before committing the tree");
>>> +				ignore_author_date(&author);
>>
>> Is this leaking the old author? I'd rather see
>>
>> 	tmp_author = ignore_author_date(author);
>> 	free(author);
>> 	author = tmp_author;
> 
> Or make sure ignore_author_date() does not leak the original, when
> it rewrites its parameter.
> 
> But I have a larger question at the higher design level.  Why are we
> passing a single string "author" around, instead of parsed and split
> fields, like <name, email, timestamp, tz> tuple?  That would allow us
> to replace only the time part a lot more easily.  Would it make the
> other parts of the code more cumbersome (I didn't check---and if
> that is the case, then that is a valid reason why we want to stick
> to the current "a single string 'author' keeps the necessary info
> for the 4-tuple" design).

It's a bit of a mess at the moment. There are places where we want a 
single author data string when calling commit_tree_extended(), and other 
places where we want to set the name, email and date in the environment 
when running 'git commit'. For the latter case we use a file which we 
read and write as the C version just follows what the shell script did. 
I think carrying round a <name, email, timestamp, tz> tuple would be the 
sensible way to go and we can build the author string when we need it. 
Doing that would hopefully eliminate having to read and write the author 
file so much. I haven't looked at how difficult it would be to change 
the existing code to do that. We should also really carry the commit 
message around in a variable and only write it to a file if a pick fails 
or we are editing the message and running 'git commit'. If we're just 
using commit_tree_extended() there is no need to be writing the message 
to a file and then reading it back in again later.

Best Wishes

Phillip

>>> +			}
>>>    			res = commit_tree(msg.buf, msg.len, cache_tree_oid,
>>>    					  NULL, &root_commit, author,
>>>    					  opts->gpg_sign);
>>> +		}
>>>      		strbuf_release(&msg);
>>>    		strbuf_release(&script);
>>> @@ -1053,6 +1087,8 @@ static int run_git_commit(struct repository *r,
>>>    		argv_array_push(&cmd.args, "--amend");
>>>    	if (opts->gpg_sign)
>>>    		argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
>>> +	if (opts->ignore_date)
>>> +		push_dates(&cmd);
>>>    	if (defmsg)
>>>    		argv_array_pushl(&cmd.args, "-F", defmsg, NULL);
>>>    	else if (!(flags & EDIT_MSG))
>>> @@ -1515,6 +1551,11 @@ static int try_to_commit(struct repository *r,
>>>      	reset_ident_date();
>>>    +	if (opts->ignore_date) {
>>> +		ignore_author_date(&author);
>>> +		free(author_to_free);
>>
>> Where is author_to_free set? We should always free the old author, see
>> above.
> 
> Or require callers to pass a free()able memory to ignore_author_date()
> and have the callee free the original?
> 

  reply	other threads:[~2019-08-14 18:48 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
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 [this message]
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=812513a6-bd83-5830-055f-7c0252e5ecd8@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).