From: "Sven M. Hallberg" Subject: [patch] preserve and show author dates To: gameoftrees@openbsd.org Date: Thu, 01 Aug 2024 19:17:19 +0200 Hi there, I've noticed that got rebase, and histedit by extension, overwrite the author timestamp with the current (committer) time. Since this was itching me, I scratched it. The patch below is my initial work to gauge if I'm on the right track here. It avoids clobbering the author timestamp in lib/worktree.c:commit_worktree() by threading it through as a separate argument. In addition, it adds support for showing a (differing) author timestamp to got log and tog. You can also see this split into some individual commits at: https://gotweb.khjk.org/?action=summary&headref=authortime&path=got.git A word on my commit_worktree() changes: I opted to keep the creation of the committer timestamp where it was, right before the call to got_object_commit_create(). Then I detect whether a different author timestamp should be applied by the presence of the optional _committer_ argument. This works fine, but seems unintuitive. The direct alternative would have been to change the interface of the function to make author the optional argument. Feedback welcome. Also, thanks a lot for got. :) -pesco PS: I have not checked thoroughly whether there are other places where author time should be preserved (cherrypick?). I have also not touched cvg or added any tests at this point. diff refs/heads/main refs/heads/authortime commit - 4b4fd6aa67065da8141a7a08bda84b95bc6ae060 commit + bfbb258b77efcda05d51b22c11c23c2b94c3cf05 blob - eedba272792e095dfe5a62504a47e88c579c69c8 blob + 717a7c74d145e56db2a81a960f1739187059af09 --- got/got.c +++ got/got.c @@ -4117,7 +4117,7 @@ done: } static char * -get_datestr(time_t *time, char *datebuf) +get_datestr(const time_t *time, char *datebuf) { struct tm mytm, *tm; char *p, *s; @@ -4405,6 +4405,18 @@ printfile(FILE *f) return NULL; } +/* print a date line unless an error occurs (or fail silently) */ +static void +print_date(const char *prefix, const time_t *t) +{ + char datebuf[26]; + char *datestr; + + datestr = get_datestr(t, datebuf); + if (datestr) + printf("%s %s UTC\n", prefix, datestr); +} + static const struct got_error * print_commit(struct got_commit_object *commit, struct got_object_id *id, struct got_repository *repo, const char *path, @@ -4415,9 +4427,8 @@ print_commit(struct got_commit_object *commit, struct { const struct got_error *err = NULL; FILE *f = NULL; - char *id_str, *datestr, *logmsg0, *logmsg, *line; - char datebuf[26]; - time_t committer_time; + char *id_str, *logmsg0, *logmsg, *line; + time_t author_time, committer_time; const char *author, *committer; char *refs_str = NULL; @@ -4447,15 +4458,20 @@ print_commit(struct got_commit_object *commit, struct id_str = NULL; free(refs_str); refs_str = NULL; - printf("from: %s\n", got_object_commit_get_author(commit)); + + /* author and committer data */ author = got_object_commit_get_author(commit); + author_time = got_object_commit_get_author_time(commit); committer = got_object_commit_get_committer(commit); + committer_time = got_object_commit_get_committer_time(commit); + printf("from: %s\n", author); + if (author_time != committer_time) + print_date("orig:", &author_time); if (strcmp(author, committer) != 0) printf("via: %s\n", committer); - committer_time = got_object_commit_get_committer_time(commit); - datestr = get_datestr(&committer_time, datebuf); - if (datestr) - printf("date: %s UTC\n", datestr); + print_date("date:", &committer_time); + + /* parent commits */ if (got_object_commit_get_nparents(commit) > 1) { const struct got_object_id_queue *parent_ids; struct got_object_qid *qid; @@ -9554,8 +9570,11 @@ cmd_commit(int argc, char *argv[]) if (error) goto done; - if (author == NULL) + if (author == NULL) { + /* got_worktree_commit() treats committer as the optional one */ author = committer; + committer = NULL; /* => author timestamp is ignored */ + } if (logmsg == NULL || strlen(logmsg) == 0) { error = get_editor(&editor); @@ -9605,8 +9624,8 @@ cmd_commit(int argc, char *argv[]) cl_arg.branch_name += 11; } cl_arg.repo_path = got_repo_get_path(repo); - error = got_worktree_commit(&id, worktree, &paths, author, committer, - allow_bad_symlinks, show_diff, commit_conflicts, + error = got_worktree_commit(&id, worktree, &paths, author, time(NULL), + committer, allow_bad_symlinks, show_diff, commit_conflicts, collect_commit_logmsg, &cl_arg, print_status, NULL, repo); if (error) { if (error->code != GOT_ERR_COMMIT_MSG_EMPTY && @@ -13844,7 +13863,7 @@ cmd_merge(int argc, char *argv[]) goto done; } else { error = got_worktree_merge_commit(&merge_commit_id, worktree, - fileindex, author, NULL, 1, branch_tip, branch_name, + fileindex, author, 0, NULL, 1, branch_tip, branch_name, allow_conflict, repo, continue_merge ? print_status : NULL, NULL); if (error) blob - 4ad750063163bd75fcdbccf958d5b7b7a72278fb blob + 01bbc9dc9ef3289c00fa880bba8f79aa56f756af --- include/got_worktree.h +++ include/got_worktree.h @@ -275,12 +275,14 @@ typedef const struct got_error *(*got_worktree_commit_ * current base commit. * An author and a non-empty log message must be specified. * The name of the committer is optional (may be NULL). + * If a committer is given, a separate author timestamp can be specified + * which is ignored otherwise. * If a path to be committed contains a symlink which points outside * of the path space under version control, raise an error unless * committing of such paths is being forced by the caller. */ const struct got_error *got_worktree_commit(struct got_object_id **, - struct got_worktree *, struct got_pathlist_head *, const char *, + struct got_worktree *, struct got_pathlist_head *, const char *, time_t, const char *, int, int, int, got_worktree_commit_msg_cb, void *, got_worktree_status_cb, void *, struct got_repository *); @@ -498,9 +500,9 @@ got_worktree_merge_branch(struct got_worktree *worktre const struct got_error * got_worktree_merge_commit(struct got_object_id **new_commit_id, struct got_worktree *worktree, struct got_fileindex *fileindex, - const char *author, const char *committer, int allow_bad_symlinks, - struct got_object_id *branch_tip, const char *branch_name, - int allow_conflict, struct got_repository *repo, + const char *author, time_t author_ts, const char *committer, + int allow_bad_symlinks, struct got_object_id *branch_tip, + const char *branch_name, int allow_conflict, struct got_repository *repo, got_worktree_status_cb status_cb, void *status_arg); /* blob - 94cb5018eb8bd148ea52e237479e027fabca1b14 blob + 5b9a648289b2a600c3cfeccfd20bff90c83e6b10 --- lib/worktree.c +++ lib/worktree.c @@ -6371,8 +6371,8 @@ commit_worktree(struct got_object_id **new_commit_id, struct got_object_id *head_commit_id, struct got_object_id *parent_id2, struct got_worktree *worktree, - const char *author, const char *committer, char *diff_path, - got_worktree_commit_msg_cb commit_msg_cb, void *commit_arg, + const char *author, time_t author_time, const char *committer, + char *diff_path, got_worktree_commit_msg_cb commit_msg_cb, void *commit_arg, got_worktree_status_cb status_cb, void *status_arg, struct got_repository *repo) { @@ -6460,8 +6460,10 @@ commit_worktree(struct got_object_id **new_commit_id, nparents++; } timestamp = time(NULL); + if (committer == NULL) + author_time = timestamp; err = got_object_commit_create(new_commit_id, new_tree_id, &parent_ids, - nparents, author, timestamp, committer, timestamp, logmsg, repo); + nparents, author, author_time, committer, timestamp, logmsg, repo); if (logmsg != NULL) free(logmsg); if (err) @@ -6577,8 +6579,8 @@ check_non_staged_files(struct got_fileindex *fileindex const struct got_error * got_worktree_commit(struct got_object_id **new_commit_id, struct got_worktree *worktree, struct got_pathlist_head *paths, - const char *author, const char *committer, int allow_bad_symlinks, - int show_diff, int commit_conflicts, + const char *author, time_t author_time, const char *committer, + int allow_bad_symlinks, int show_diff, int commit_conflicts, got_worktree_commit_msg_cb commit_msg_cb, void *commit_arg, got_worktree_status_cb status_cb, void *status_arg, struct got_repository *repo) @@ -6691,7 +6693,7 @@ got_worktree_commit(struct got_object_id **new_commit_ } err = commit_worktree(new_commit_id, &commitable_paths, - head_commit_id, NULL, worktree, author, committer, + head_commit_id, NULL, worktree, author, author_time, committer, (diff_path && cc_arg.diff_header_shown) ? diff_path : NULL, commit_msg_cb, commit_arg, status_cb, status_arg, repo); if (err) @@ -7339,6 +7341,7 @@ rebase_commit(struct got_object_id **new_commit_id, /* NB: commit_worktree will call free(logmsg) */ err = commit_worktree(new_commit_id, &commitable_paths, head_commit_id, NULL, worktree, got_object_commit_get_author(orig_commit), + got_object_commit_get_author_time(orig_commit), committer ? committer : got_object_commit_get_committer(orig_commit), NULL, collect_rebase_commit_msg, logmsg, rebase_status, NULL, repo); @@ -8651,11 +8654,10 @@ done: const struct got_error * got_worktree_merge_commit(struct got_object_id **new_commit_id, struct got_worktree *worktree, struct got_fileindex *fileindex, - const char *author, const char *committer, int allow_bad_symlinks, - struct got_object_id *branch_tip, const char *branch_name, - int allow_conflict, struct got_repository *repo, + const char *author, time_t author_time, const char *committer, + int allow_bad_symlinks, struct got_object_id *branch_tip, + const char *branch_name, int allow_conflict, struct got_repository *repo, got_worktree_status_cb status_cb, void *status_arg) - { const struct got_error *err = NULL, *sync_err; struct got_pathlist_head commitable_paths; @@ -8708,8 +8710,9 @@ got_worktree_merge_commit(struct got_object_id **new_c mcm_arg.worktree = worktree; mcm_arg.branch_name = branch_name; err = commit_worktree(new_commit_id, &commitable_paths, - head_commit_id, branch_tip, worktree, author, committer, NULL, - merge_commit_msg_cb, &mcm_arg, status_cb, status_arg, repo); + head_commit_id, branch_tip, worktree, author, author_time, + committer, NULL, merge_commit_msg_cb, &mcm_arg, status_cb, + status_arg, repo); if (err) goto done; blob - c824aaf0100a26fa4d112f62f5136272c5c82bbc blob + aa8c37ce5f9af6c3b3fdf320c95dd8316334d667 --- regress/cmdline/rebase.sh +++ regress/cmdline/rebase.sh @@ -952,8 +952,8 @@ test_rebase_preserves_logmsg() { echo "modified alpha on branch" > $testroot/repo/alpha git_commit $testroot/repo -m "modified alpha on newbranch" - (cd $testroot/repo && got log -c newbranch -l2 | grep -v ^date: \ - > $testroot/log.expected) + (cd $testroot/repo && got log -c newbranch -l2 | \ + egrep -v '^(date|orig):' > $testroot/log.expected) local orig_commit1=`git_show_parent_commit $testroot/repo` local orig_commit2=`git_show_head $testroot/repo` @@ -986,8 +986,8 @@ test_rebase_preserves_logmsg() { return 1 fi - (cd $testroot/wt && got log -c newbranch -l2 | grep -v ^date: \ - > $testroot/log) + (cd $testroot/wt && got log -c newbranch -l2 | \ + egrep -v '^(date|orig):' > $testroot/log) ed -s $testroot/log.expected <<-EOF ,s/$orig_commit1/$new_commit1/ ,s/$orig_commit2/$new_commit2/ blob - 674de2c51543e1af563af32b17a4d609805167ec blob + 5289fe88e2afd9cf2bdc043d4722c21fa4843071 --- tog/tog.c +++ tog/tog.c @@ -571,7 +571,7 @@ struct tog_help_view_state { KEY_("R", "Open ref view of all repository references"), \ KEY_("T", "Display tree view of the repository from the selected" \ " commit"), \ - KEY_("@", "Toggle between displaying author and committer name"), \ + KEY_("@", "Toggle between displaying author and committer data"), \ KEY_("&", "Open prompt to enter term to limit commits displayed"), \ KEY_("C-g Backspace", "Cancel current search or log operation"), \ KEY_("C-l", "Reload the log view with new commits in the repository"), \ @@ -2441,7 +2441,7 @@ draw_commit(struct tog_view *view, struct commit_queue int col, limit, scrollx, logmsg_x; const int avail = view->ncols, marker_column = author_display_cols + 1; struct tm tm; - time_t committer_time; + time_t timestamp; struct tog_color *tc; struct got_reflist_head *refs; @@ -2456,8 +2456,11 @@ draw_commit(struct tog_view *view, struct commit_queue return got_error_set_errno(rc, "pthread_cond_wait"); } - committer_time = got_object_commit_get_committer_time(commit); - if (gmtime_r(&committer_time, &tm) == NULL) + if (s->use_committer) + timestamp = got_object_commit_get_committer_time(commit); + else + timestamp = got_object_commit_get_author_time(commit); + if (gmtime_r(×tamp, &tm) == NULL) return got_error_from_errno("gmtime_r"); if (strftime(datebuf, sizeof(datebuf), "%F ", &tm) == 0) return got_error(GOT_ERR_NO_SPACE); @@ -5033,7 +5036,7 @@ draw_file(struct tog_view *view, const char *header) } static char * -get_datestr(time_t *time, char *datebuf) +get_datestr(const time_t *time, char *datebuf) { struct tm mytm, *tm; char *p, *s; @@ -5123,16 +5126,41 @@ cat_diff(FILE *dst, FILE *src, struct got_diff_line ** } static const struct got_error * +write_date(struct got_diff_line **lines, size_t *nlines, + const char *prefix, const time_t *t, FILE *outfile, off_t *outoff) +{ + const struct got_error *err; + char datebuf[26], *datestr; + int n; + + datestr = get_datestr(t, datebuf); + if (datestr == NULL) + return NULL; /* silently ignored */ + + n = fprintf(outfile, "%s %s UTC\n", prefix, datestr); + if (n < 0) { + err = got_error_from_errno("fprintf"); + return err; + } + *outoff += n; + err = add_line_metadata(lines, nlines, *outoff, + GOT_DIFF_LINE_DATE); + if (err) + return err; + + return NULL; +} + +static const struct got_error * write_commit_info(struct got_diff_line **lines, size_t *nlines, struct got_object_id *commit_id, struct got_reflist_head *refs, struct got_repository *repo, int ignore_ws, int force_text_diff, struct got_diffstat_cb_arg *dsa, FILE *outfile) { const struct got_error *err = NULL; - char datebuf[26], *datestr; struct got_commit_object *commit; char *id_str = NULL, *logmsg = NULL, *s = NULL, *line; - time_t committer_time; + time_t author_time, committer_time; const char *author, *committer; char *refs_str = NULL; struct got_pathlist_entry *pe; @@ -5168,19 +5196,25 @@ write_commit_info(struct got_diff_line **lines, size_t if (err) goto done; - n = fprintf(outfile, "from: %s\n", - got_object_commit_get_author(commit)); - if (n < 0) { - err = got_error_from_errno("fprintf"); - goto done; - } - outoff += n; - err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_AUTHOR); - if (err) - goto done; - author = got_object_commit_get_author(commit); + author_time = got_object_commit_get_author_time(commit); committer = got_object_commit_get_committer(commit); + committer_time = got_object_commit_get_committer_time(commit); + n = fprintf(outfile, "from: %s\n", author); + if (n < 0) { + err = got_error_from_errno("fprintf"); + goto done; + } + outoff += n; + err = add_line_metadata(lines, nlines, outoff, GOT_DIFF_LINE_AUTHOR); + if (err) + goto done; + if (author_time != committer_time) { + err = write_date(lines, nlines, "orig:", &author_time, outfile, + &outoff); + if (err) + goto done; + } if (strcmp(author, committer) != 0) { n = fprintf(outfile, "via: %s\n", committer); if (n < 0) { @@ -5193,20 +5227,10 @@ write_commit_info(struct got_diff_line **lines, size_t if (err) goto done; } - committer_time = got_object_commit_get_committer_time(commit); - datestr = get_datestr(&committer_time, datebuf); - if (datestr) { - n = fprintf(outfile, "date: %s UTC\n", datestr); - if (n < 0) { - err = got_error_from_errno("fprintf"); - goto done; - } - outoff += n; - err = add_line_metadata(lines, nlines, outoff, - GOT_DIFF_LINE_DATE); - if (err) - goto done; - } + err = write_date(lines, nlines, "date:", &committer_time, outfile, + &outoff); + if (err) + goto done; if (got_object_commit_get_nparents(commit) > 1) { const struct got_object_id_queue *parent_ids; struct got_object_qid *qid;