git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/6] cherry-pick: Use refs infrastructure for CHERRY_PICK_HEAD

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 25, 2015, 16:41 UTC
Message-ID
<xmqqvbebzqns.fsf@gitster.dls.corp.google.com>
In-Reply-To
<1435173388-8346-3-git-send-email-dturner@twopensource.com>
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.
Show 6 quoted lines
>  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.

Show 7 quoted lines
>  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.
Show 14 quoted lines
> 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.

Show 9 quoted lines
> @@ -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.

Show 46 quoted lines
> 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.

Previous: David TurnerNext: David Turner
Message 5 of 16 in “refs backend preamble”
  1. 0/6 refs backend preambleDavid Turner, Jun 24, 2015
  2. 1/6 refs.c: add an err argument to log_ref_setupDavid Turner, Jun 24, 2015
  3. Junio C HamanoJun 25, 2015
  4. 2/6 cherry-pick: Use refs infrastructure for CHERRY_PICK_HEADDavid Turner, Jun 24, 2015
  5. Junio C HamanoJun 25, 2015
  6. 3/6 bisect: use refs infrastructure for BISECT_STARTDavid Turner, Jun 24, 2015
  7. Junio C HamanoJun 25, 2015
  8. David TurnerJun 25, 2015
  9. 4/6 refs: replace log_ref_write with create_reflogDavid Turner, Jun 24, 2015
  10. Junio C HamanoJun 25, 2015
  11. 5/6 git-reflog: add create and exists functionsDavid Turner, Jun 24, 2015
  12. Junio C HamanoJun 25, 2015
  13. Junio C HamanoJun 25, 2015
  14. David TurnerJun 25, 2015
  15. 6/6 git-stash: use git-reflog instead of creating filesDavid Turner, Jun 24, 2015
  16. Junio C HamanoJun 25, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.