[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] Changed timestamp behavior of options -c/-C/--amend
From:       Erick Mattos <erick.mattos () gmail ! com>
Date:       2009-10-30 21:22:46
Message-ID: 55bacdd30910301422w2a2fa71cw74bd71d9b9383b85 () mail ! gmail ! com
[Download RAW message or body]

2009/10/30 Jeff King <peff@peff.net>:
> On Fri, Oct 30, 2009 at 05:36:34PM -0200, Erick Mattos wrote:
>
>> Anyway this update creates new options for choosing the source timestamp
>> or a new one.  And set as default for -c option (editing one) to take a
>> new timestamp and for -C option the source timestamp.  That is because
>> we are normally using the source as template when we we are editing and
>> as a correction when we are just copying it.
>>
>> Those options are also useful for --amend option which is by default
>> behaving the same.
>
> Thanks, this is something I have been wanting. I have always thought
> that --amend should give a new timestamp, so that while I'm fixing up
> commits via "rebase -i" the commits end up in correct date order.

You are welcome!


> Your patch seems to always use the old timestamp for -C, the new one for
> -c, and the old one for --amend. I would want it always for --amend.

About --amend: I didn't want to change the actual behavior and as a
matter of fact it means only adding the --new-timestamp option when
needed anyway.


> I talked with Shawn about this at the GitTogether; his counter-argument
> was that many people in maintainer roles will be amending or rebasing
> just to do little things, like marking Signed-off-by, and that the date
> should remain the same.
>
> So my suspicion is that there are some people who almost always want
> --new-timestamp, and some people who almost always want --old-timestamp,
> depending on their usual workflow. In which case a config option
> probably makes the most sense (but keeping the command-line to override
> the config, of course).

As you know you will find people defending both sides.  I am one that
prefers --amend the way it is now.  I really think that having an
option to change it is enough to keep peace.

I don't see a need for more changes in config because it would be imho
more complexity for a small need.

Of course you can do some alias in bash for setting mentioned
functionality up! ;-)


>> ---
>>  Documentation/git-commit.txt |   10 ++++++++--
>>  builtin-commit.c             |   15 ++++++++++++---
>>  2 files changed, 20 insertions(+), 5 deletions(-)
>
> Tests?

Yes indeed.  It is a change I am using which I thought it would be
useful for other people.  The code is quite simple too.  Simplicity
always lead to less possibility of bugs.  But yes, I had tested it.


>>       Take an existing commit object, and reuse the log message
>> -     and the authorship information (including the timestamp)
>> -     when creating the commit.
>> +     and the authorship information when creating the commit.
>> +     By default, timestamp is taken from specified commit unless
>> +     option --new-timestamp is included.
>
> We should clarify that this is the _author_ timestamp. The committer
> timestamp is always updated. Yes, it is talking about authorship
> information in the previous sentence, but at least I had to read it
> a few times to figure that out. Plus there are some minor English
> tweaks needed, so maybe:

I see your point but I do not think the way it is is confusing.  Of
course we will be talking about taste so I let it pass.  Anyway who
would think about or want to change the commiter timestamp?  Maybe a
fraudster!...  :-1


>  and the authorship information when creating the commit.
>  By default, the author timestamp is taken from the specified commit
>  unless the option --new-timestamp is used.
>
>>  -c <commit>::
>>  --reedit-message=<commit>::
>>       Like '-C', but with '-c' the editor is invoked, so that
>>       the user can further edit the commit message.
>> +     By default, timestamp is recalculated and not taken from
>> +     specified commit unless option --old-timestamp is included.
>
> Ditto:
>
>  By default, the author timestamp is recalculated and not taken from
>  the specified commit unless the option --old-timestamp is used.
>
>> @@ -134,6 +138,8 @@ OPTIONS
>>       current tip -- if it was a merge, it will have the parents of
>>       the current tip as parents -- so the current top commit is
>>       discarded.
>> +     By default, timestamp is taken from latest commit unless option
>> +     --new-timestamp is included.
>
> And:
>
>  By default, the author timestamp is taken from the latest commit
>  unless the option --new-timestamp is included.

As previously said I wouldn't change the text but anyway...


>> +     if (old_timestamp && new_timestamp)
>> +             die("You can not use --old-timesamp and --new-timestamp together.");
>
> Typo: s/samp/stamp/
>
> But this mutual exclusivity implies to me that the option should
> probably be "--timestamp=old|new".
>
> -Peff
>

Now it is a matter of taste again...  I prefer the options as I set up
because it is more the way other options are used so it is more
intuitive.  We use very little --'option'='something' using git.

Thanks for all your comments.  I have appreciated them very much.

The differences in opinion between us about the little details of this
update is not really relevant because we agree in the need of the
customization proposed.

I have really presented it because I thought it would be useful.

So I think the maintainer can accept it as he judges better.

One of the beauties of Free Software is: even if people don't agree on
a subject one can always compile and use the way one wants.  So
everybody gets satisfied!

Thank you very much again.

Best regards.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]