From: "Sven M. Hallberg" Subject: Re: [patch] preserve and show author dates To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Sat, 03 Aug 2024 22:22:36 +0200 Stefan Sperling on Fri, Aug 02 2024: > No objections to your patch from me (not committing it myself; due to a > wrist injury I have a bit of energy to handle email, but not much > else). I noticed that I managed to mix up the order of the headers (tired eyes?!) and spotted a mismatch between the (new) argument names in worktree.c and got_worktree.h. Revised patch below. >> > In cases where histedit or cherrypick fold multiple commits into >> > a single commit, preserving info about potentially multiple authors >> > of the original commits becomes impossible. What should happen then? >> >> That is a good question. However, it applies equally to the author name. >> I haven't checked what got or git do in that case. So I went and checked... For histedit, got uses whatever author data is in the last commit (the one being folded into), since 'fold' just applies changes to the worktree and otherwise acts like 'skip' wrt. the commit they came from. Git does it the other way around: 'pick' the first commit and 'squash' the rest onto it; author data is taken from the "picked" commit, presumably including the timestamp (?). The question does not apply to got cherrypick because that, too, simply applies changes to the worktree (i.e. does not create a commit). As for git cherry-pick, it creates a new commit from each of its commit arguments. I would assume it transfers the author data directly. > I wish there was a better way, but for compatiblity reasons I want to > refrain from adding custom extensions to the format to fix such issues. Just idle thoughts for whatever they're worth: The design decision on git's part seems pretty reasonable to me. I feel that it preserves important information for the common case while not going overboard with complexity. YMMV -p diff refs/heads/main refs/heads/authortime commit - 4b4fd6aa67065da8141a7a08bda84b95bc6ae060 commit + 49b650bb078a73e7a05703caa4e47cdae61ae1fe blob - eedba272792e095dfe5a62504a47e88c579c69c8 blob + c000d1047d7b2bd91e7015e05e3443279216305b --- 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 (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); + if (author_time != committer_time) + print_date("orig:", &author_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 + 6801a4bd92ef6dec2d02f8dd54a39617c1b5845e --- 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_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); /* 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 + 6f62269b5db678a60f947bec3954fb82ba0cd2a4 --- 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,20 @@ 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 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); + 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 (strcmp(author, committer) != 0) { n = fprintf(outfile, "via: %s\n", committer); if (n < 0) { @@ -5193,20 +5222,18 @@ 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); + err = write_date(lines, nlines, "date:", &committer_time, outfile, + &outoff); + if (err) + goto done; + if (author_time != committer_time) { + err = write_date(lines, nlines, "orig:", &author_time, outfile, + &outoff); if (err) goto done; } + + /* parent commits */ if (got_object_commit_get_nparents(commit) > 1) { const struct got_object_id_queue *parent_ids; struct got_object_qid *qid;