From: Junio C Hamano <gitster@pobox.com>
To: David Turner <dturner@twopensource.com>
Cc: git@vger.kernel.org, mhagger@alum.mit.edu
Subject: Re: [PATCH 2/6] cherry-pick: Use refs infrastructure for CHERRY_PICK_HEAD
Date: Thu, 25 Jun 2015 09:41:43 -0700 [thread overview]
Message-ID: <xmqqvbebzqns.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1435173388-8346-3-git-send-email-dturner@twopensource.com> (David Turner's message of "Wed, 24 Jun 2015 15:16:24 -0400")
David Turner <dturner@twopensource.com> 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.
next prev parent reply other threads:[~2015-06-25 16:41 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-24 19:16 [PATCH 0/6] refs backend preamble David Turner
2015-06-24 19:16 ` [PATCH 1/6] refs.c: add an err argument to log_ref_setup David Turner
2015-06-25 16:15 ` Junio C Hamano
2015-06-24 19:16 ` [PATCH 2/6] cherry-pick: Use refs infrastructure for CHERRY_PICK_HEAD David Turner
2015-06-25 16:41 ` Junio C Hamano [this message]
2015-06-24 19:16 ` [PATCH 3/6] bisect: use refs infrastructure for BISECT_START David Turner
2015-06-25 16:52 ` Junio C Hamano
2015-06-25 20:29 ` David Turner
2015-06-24 19:16 ` [PATCH 4/6] refs: replace log_ref_write with create_reflog David Turner
2015-06-25 16:54 ` Junio C Hamano
2015-06-24 19:16 ` [PATCH 5/6] git-reflog: add create and exists functions David Turner
2015-06-25 17:18 ` Junio C Hamano
2015-06-25 18:35 ` Junio C Hamano
2015-06-25 20:31 ` David Turner
2015-06-24 19:16 ` [PATCH 6/6] git-stash: use git-reflog instead of creating files David Turner
2015-06-25 13:19 ` [PATCH 0/6] refs backend preamble Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqvbebzqns.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox.com \
--cc=dturner@twopensource.com \
--cc=git@vger.kernel.org \
--cc=mhagger@alum.mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).