David Turner <dturner@xxxxxxxxxxxxxxxx> writes:
> Subject: Re: [PATCH 2/6] cherry-pick: Use refs infrastructure for CHERRY_PICK_HEAD
> Also use refs infrastructure for REVERT_HEAD. These refs
> need to go through the refs backend, since some code
> assumes that they can be read as refs.
cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
Instead of directly writing to and reading from files in
$GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD
and REVERT_HEAD.
> void remove_branch_state(void)
> {
> - unlink(git_path("CHERRY_PICK_HEAD"));
> - unlink(git_path("REVERT_HEAD"));
> + delete_ref("CHERRY_PICK_HEAD", NULL, REF_NODEREF | REF_NO_REFLOG);
> + delete_ref("REVERT_HEAD", NULL, REF_NODEREF | REF_NO_REFLOG);
Interesting. There is a separate question about NO_REFLOG I'll
discuss in more detail later. But no-deref puzzled me. They should
not be symbolic, but if somebody by mistake made a symbolic one, we
won't accidentally remove another unrelated one through them, so
that bit is a good thing to have.
> static void determine_whence(struct wt_status *s)
> {
> + unsigned char unused[20];
> if (file_exists(git_path("MERGE_HEAD")))
> whence = FROM_MERGE;
> - else if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
> + else if (!read_ref("CHERRY_PICK_HEAD", unused)) {
I would have expected that you would use ref_exists() here.
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 366f0bc..5e27a34 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -415,9 +415,9 @@ __git_ps1 ()
> fi
> elif [ -f "$g/MERGE_HEAD" ]; then
> r="|MERGING"
> - elif [ -f "$g/CHERRY_PICK_HEAD" ]; then
> + elif git rev-parse --quiet --verify "CHERRY_PICK_HEAD" >/dev/null; then
> r="|CHERRY-PICKING"
> - elif [ -f "$g/REVERT_HEAD" ]; then
> + elif git rev-parse --quiet --verify "REVERT_HEAD" >/dev/null; then
> r="|REVERTING"
Functionality-wise this may be OK but at some point we might want to
have a native way to produce $r with a single execution of a binary.
> @@ -429,8 +429,9 @@ __git_ps1 ()
> # symlink symbolic ref
> b="$(git symbolic-ref HEAD 2>/dev/null)"
> else
> - local head=""
> - if ! __git_eread "$g/HEAD" head; then
> + local head
> + head="ref: $(git symbolic-ref HEAD 2>/dev/null)" || head=$(git rev-parse HEAD)
> + if [ -z "$head" ]; then
This is questionable; before the pre-context of this hunk is a check
for "HEAD" inside $GIT_DIR on the filesystem. Besides, the "theme"
of this patch is about CHERRY_PICK_HEAD and REVERT_HEAD; it does not
justify to touch things that deal with HEAD in the same patch.
> diff --git a/refs.c b/refs.c
> index b34a54a..c1a563f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2979,7 +2979,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
> const unsigned char *sha1, struct strbuf* err);
> static int commit_ref_update(struct ref_lock *lock,
> const unsigned char *sha1, const char *logmsg,
> - struct strbuf *err);
> + struct strbuf *err, int flags);
>
> int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg)
> {
> @@ -3041,7 +3041,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
> hashcpy(lock->old_oid.hash, orig_sha1);
>
> if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
> - commit_ref_update(lock, orig_sha1, logmsg, &err)) {
> + commit_ref_update(lock, orig_sha1, logmsg, &err, 0)) {
> error("unable to write current sha1 into %s: %s", newrefname, err.buf);
> strbuf_release(&err);
> goto rollback;
> @@ -3060,7 +3060,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
> flag = log_all_ref_updates;
> log_all_ref_updates = 0;
> if (write_ref_to_lockfile(lock, orig_sha1, &err) ||
> - commit_ref_update(lock, orig_sha1, NULL, &err)) {
> + commit_ref_update(lock, orig_sha1, NULL, &err, 0)) {
> error("unable to write current sha1 into %s: %s", oldrefname, err.buf);
> strbuf_release(&err);
> }
> @@ -3291,12 +3291,13 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
> */
> static int commit_ref_update(struct ref_lock *lock,
> const unsigned char *sha1, const char *logmsg,
> - struct strbuf *err)
> + struct strbuf *err, int flags)
> {
> clear_loose_ref_cache(&ref_cache);
> - if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0 ||
> - (strcmp(lock->ref_name, lock->orig_ref_name) &&
> - log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0)) {
> + if (!(flags & REF_NO_REFLOG) &&
> + (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0 ||
> + (strcmp(lock->ref_name, lock->orig_ref_name) &&
> + log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0))) {
What is this change about? I suspect this should be a separate
preparatory commit to allow callers to skip reflog update.
But more importantly, I do not think that it is a good idea in the
first place to allow callers to selectively say "for this update, do
not write a reflog entry". If you do three ref-updates on the same
ref in a sequence (e.g. imagine you created a single strand of
pearls consisting of three commits) but passed ref-no-reflog only
for the second update, wouldn't that make your reflog entries
inconsistent?
REF_NO_REFLOG functionality that seletively allows updates to be
skipped should *NOT* exist, I think.
In other words, I think "do updates logged" should be a property of
each ref. If changes to a ref is to be logged, all changes to it
must be logged. If changes to a ref is not to be logged, no changes
to it must be logged.
The core.logAllRefUpdates configuration may appear to throw a monkey
wrench to that if its name is taken literally, as your creation of
CHECK_HEAD may get logged, which is not what you want. But we are
under no obligation to obey core.logAllRefUpdates when updating
CHECK_HEAD or REVERT_HEAD.
core.logAllRefUpdates::
Enable the reflog. Updates to a ref <ref> is logged to the file
"$GIT_DIR/logs/<ref>", by appending the new and old
SHA-1, the date/time and the reason of the update, but
only when the file exists. If this configuration
variable is set to true, missing "$GIT_DIR/logs/<ref>"
file is automatically created for branch heads (i.e. under
refs/heads/), remote refs (i.e. under refs/remotes/),
note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
It is fairly clear that we promise not to auto-vivify reflogs for
CHERRY_PICK_HEAD and REVERT_HEAD from this description.
And if some users create $GIT_DIR/logs/CHERRY_PICK_HEAD to enable
reflog for CHERRY_PICK_HEAD, then it is not our business to refuse
logging the change just to speed things up a bit.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html