Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rebase -i: support --ignore-date #4

Closed
wants to merge 3 commits into from

Conversation

@r1walz
Copy link
Owner

commented Jul 18, 2019

Let's review this one.

@tgummerer
Copy link

left a comment

Thanks! Quite a lot to unpack in that single line there, though I think it's relatively easy to fix :)

Nice work, looking forward to reading the docs and the tests for this as well.

sequencer.c Outdated Show resolved Hide resolved
sequencer.c Show resolved Hide resolved
sequencer.c Show resolved Hide resolved
@newren
Copy link

left a comment

Only two small comments other than what Thomas found.

builtin/rebase.c Outdated Show resolved Hide resolved
sequencer.c Show resolved Hide resolved

@r1walz r1walz force-pushed the rb-cdia branch from 33cfc0f to 70eeea5 Jul 24, 2019

@r1walz r1walz force-pushed the rb-cdia branch from 70eeea5 to 8a5ca26 Aug 4, 2019

@r1walz r1walz force-pushed the rb--ignore-date branch 2 times, most recently from ba5ffcd to ca585f3 Aug 4, 2019

@r1walz r1walz marked this pull request as ready for review Aug 4, 2019

Documentation/git-rebase.txt Show resolved Hide resolved
Documentation/git-rebase.txt Outdated Show resolved Hide resolved
sequencer.c Outdated Show resolved Hide resolved
t/t3433-rebase-options-compatibility.sh Show resolved Hide resolved
sequencer.c Outdated Show resolved Hide resolved

@r1walz r1walz force-pushed the rb--ignore-date branch from ca585f3 to 838df37 Aug 5, 2019

r1walz added some commits Aug 4, 2019

rebase -i: support --ignore-date
rebase am already has this flag to "lie" about the author date
by changing it to the committer (current) date. Let's add the same
for interactive machinery.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
sequencer: rename amend_author to author_to_free
The purpose of amend_author was to free() the malloc()'d string
obtained from get_author() when AMEND_MSG flag was set. But now,  we
also use it to free() the new parsed author so, the name is no longer
relevant. Rename it to something meaningful.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
rebase: add --author-date-is-committer-date
Commit 554377d ("rebase -i: support --ignore-date", 2019-08-05)
introduced --ignore-date flag to interactive rebase, but the name
is actually very vague in context of rebase -i since there are two
dates we can work with. Add an alias to convey the precise purpose.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

@r1walz r1walz force-pushed the rb-cdia branch from 8a5ca26 to 28a54cd Aug 5, 2019

@r1walz r1walz force-pushed the rb--ignore-date branch 2 times, most recently from 385fb4a to 1b73405 Aug 5, 2019

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Aug 6, 2019

@tgummerer @newren Let's have a final call before sending this to the ML.

@@ -1425,7 +1447,7 @@ static int try_to_commit(struct repository *r,
if (parse_head(r, &current_head))
return -1;
if (!(flags & AMEND_MSG) && opts->committer_date_is_author_date &&
setenv_committer_date_to_author_date())
!opts->ignore_date && setenv_committer_date_to_author_date())

This comment has been minimized.

Copy link
@newren

newren Aug 6, 2019

What if someone passes both --committer-date-is-author-date and --ignore-date? Do they expect author date and committer date to get swapped? Right now, this appears to make --ignore-date override --committer-date-is-author-date -- perhaps we should error out instead? (Or maybe this is fine as-is because git-am also surprisingly just makes --ignore-date silently override --committer-date-is-author-date, without documenting it, so you've at least matched the behavior elsewhere...)

Not sure you need to do anything here, especially since you do match git-am behavior, but it might be worth pointing out in your cover letter.

@newren
Copy link

left a comment

Looks good, but (as a very micro nit:) you have a double space after a comma in the commit message; might be nice to clean that up.

@newren

newren approved these changes Aug 6, 2019

Copy link

left a comment

Wahoo! I like it.

@tgummerer
Copy link

left a comment

Commit 554377d ("rebase -i: support --ignore-date", 2019-08-05)

Unfortunately you can't refer to a commit like that when it's part of a patch series, as the commit ID will be different once Junio actually applies the earlier patch. Instead, the patch can just be referred as "a previous patch" or something like that.

I would also make what you have as the second commit in the series ("sequencer: rename amend_author to author_to_free") the first commit, as it's really a preparatory step that makes the commit after it easier to read. Right now it's more of an "oops, we should have named this differently, let's do it now" commit, which we generally prefer not to do in this project.

Other than those two nits the code looks good to me 🎉

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Aug 6, 2019

--- sent to the Mailing List ---

@r1walz r1walz closed this Aug 6, 2019

@r1walz r1walz deleted the rb--ignore-date branch Aug 6, 2019

@r1walz r1walz restored the rb--ignore-date branch Aug 6, 2019

@r1walz r1walz deleted the rb--ignore-date branch Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.