[PATCH v5 1/5] refs: introduce pseudoref and per-worktree ref concepts

[PATCH v5 1/5] refs: introduce pseudoref and per-worktree ref concepts

[David Turner]

Add glossary entries for both concepts.

Pseudorefs and per-worktree refs do not yet have special handling,
because the files refs backend already handles them correctly.  Later,
we will make the LMDB backend call out to the files backend to handle
per-worktree refs.

Signed-off-by: David Turner <dturner@twopensource.com>
---

This version squashes in Junio's glossary and capitalization
corrections, and corrects the spelling of pseudoref (thanks to Johan
Herland).

---

 Documentation/glossary-content.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index ab18f4b..8c6478b 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -411,6 +411,27 @@ exclude;;
 	core Git. Porcelains expose more of a <<def_SCM,SCM>>
 	interface than the <<def_plumbing,plumbing>>.
 
+[[def_per_worktree_ref]]per-worktree ref::
+	Refs that are per-<<def_working_tree,worktree>>, rather than
+	global.  This is presently only <<def_HEAD,HEAD>>, but might
+	later include other unusual refs.
+
+[[def_pseudoref]]pseudoref::
+	Pseudorefs are a class of files under `$GIT_DIR` which behave
+	like refs for the purposes of rev-parse, but which are treated
+	specially by git.  Pseudorefs both have names that are all-caps,
+	and always start with a line consisting of a
+	<<def_SHA1,SHA-1>> followed by whitespace.  So, HEAD is not a
+	pseudoref, because it is sometimes a symbolic ref.  They might
+	optionally contain some additional data.  `MERGE_HEAD` and
+	`CHERRY_PICK_HEAD` are examples.  Unlike
+	<<def_per_worktree_ref,per-worktree refs>>, these files cannot
+	be symbolic refs, and never have reflogs.  They also cannot be
+	updated through the normal ref update machinery.  Instead,
+	they are updated by directly writing to the files.  However,
+	they can be read as if they were refs, so `git rev-parse
+	MERGE_HEAD` will work.
+
 [[def_pull]]pull::
 	Pulling a <<def_branch,branch>> means to <<def_fetch,fetch>> it and
 	<<def_merge,merge>> it.  See also linkgit:git-pull[1].
-- 
2.0.4.315.gad8727a-twtrsrc

[PATCH v5 2/5] refs: add ref_type function

[David Turner]

Add a function ref_type, which categorizes refs as per-worktree,
pseudoref, or normal ref.

Later, we will use this in refs.c to treat pseudorefs specially.
Alternate ref backends may use it to treat both pseudorefs and
per-worktree refs differently.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 refs.c | 26 ++++++++++++++++++++++++++
 refs.h |  8 ++++++++
 2 files changed, 34 insertions(+)

diff --git a/refs.c b/refs.c
index 0b96ece..0f87884 100644
--- a/refs.c
+++ b/refs.c
@@ -2848,6 +2848,32 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
 	return 0;
 }
 
+static int is_per_worktree_ref(const char *refname)
+{
+	return !strcmp(refname, "HEAD");
+}
+
+static int is_pseudoref_syntax(const char *refname)
+{
+	const char *c;
+
+	for (c = refname; *c; c++) {
+		if (!isupper(*c) && *c != '-' && *c != '_')
+			return 0;
+	}
+
+	return 1;
+}
+
+enum ref_type ref_type(const char *refname)
+{
+	if (is_per_worktree_ref(refname))
+		return REF_TYPE_PER_WORKTREE;
+	if (is_pseudoref_syntax(refname))
+		return REF_TYPE_PSEUDOREF;
+       return REF_TYPE_NORMAL;
+}
+
 int delete_ref(const char *refname, const unsigned char *old_sha1,
 	       unsigned int flags)
 {
diff --git a/refs.h b/refs.h
index e4e46c3..dca4fb5 100644
--- a/refs.h
+++ b/refs.h
@@ -445,6 +445,14 @@ extern int parse_hide_refs_config(const char *var, const char *value, const char
 
 extern int ref_is_hidden(const char *);
 
+enum ref_type {
+	REF_TYPE_PER_WORKTREE,
+	REF_TYPE_PSEUDOREF,
+	REF_TYPE_NORMAL,
+};
+
+enum ref_type ref_type(const char *refname);
+
 enum expire_reflog_flags {
 	EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
 	EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v5 2/5] refs: add ref_type function

[Duy Nguyen]

On Fri, Jul 31, 2015 at 1:06 PM, David Turner <dturner@twopensource.com> wrote:
> Add a function ref_type, which categorizes refs as per-worktree,
> pseudoref, or normal ref.

For per-worktree refs, you probably should follow common_list[] in
path.c because that's how file-based ref namespace is splitted between
per-repo and per-worktree, even though just as simple as "everything
outside refs/ is per-worktree" (with an exception of NOTES_MERGE_REF,
which should be on the list as well). At least the two should be
aligned so that the default file-based backend works the same way as
new backends.

Going further, I think you need to pass the "worktree identifier" to
ref backend, at least in ref_transaction_begin_fn. Each backend is
free to store per-worktree refs however it wants. Of course if I ask
for refs/foo of worktree A, you should not return me refs/foo of
worktree B. ref_transaction_begin_fn can return a fault code if it
does not support multiple worktrees, which is fine.

> Later, we will use this in refs.c to treat pseudorefs specially.
> Alternate ref backends may use it to treat both pseudorefs and
> per-worktree refs differently.

I'm not so sure that this can't be hidden behind backends and they can
have total control on falling back to file-based, or store them in
some secondary storage. I haven't re-read your discussion with Junio
yet (only skimmed through long ago) so I may be missing some important
points.

>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  refs.c | 26 ++++++++++++++++++++++++++
>  refs.h |  8 ++++++++
>  2 files changed, 34 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index 0b96ece..0f87884 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2848,6 +2848,32 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>         return 0;
>  }
>
> +static int is_per_worktree_ref(const char *refname)
> +{
> +       return !strcmp(refname, "HEAD");
> +}
> +
> +static int is_pseudoref_syntax(const char *refname)
> +{
> +       const char *c;
> +
> +       for (c = refname; *c; c++) {
> +               if (!isupper(*c) && *c != '-' && *c != '_')
> +                       return 0;
> +       }
> +
> +       return 1;
> +}
> +
> +enum ref_type ref_type(const char *refname)
> +{
> +       if (is_per_worktree_ref(refname))
> +               return REF_TYPE_PER_WORKTREE;
> +       if (is_pseudoref_syntax(refname))
> +               return REF_TYPE_PSEUDOREF;
> +       return REF_TYPE_NORMAL;
> +}
> +
>  int delete_ref(const char *refname, const unsigned char *old_sha1,
>                unsigned int flags)
>  {
> diff --git a/refs.h b/refs.h
> index e4e46c3..dca4fb5 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -445,6 +445,14 @@ extern int parse_hide_refs_config(const char *var, const char *value, const char
>
>  extern int ref_is_hidden(const char *);
>
> +enum ref_type {
> +       REF_TYPE_PER_WORKTREE,
> +       REF_TYPE_PSEUDOREF,
> +       REF_TYPE_NORMAL,
> +};
> +
> +enum ref_type ref_type(const char *refname);
> +
>  enum expire_reflog_flags {
>         EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
>         EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
> --
> 2.0.4.315.gad8727a-twtrsrc
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Duy

Re: [PATCH v5 2/5] refs: add ref_type function

[David Turner]

On Mon, 2015-08-03 at 20:55 +0700, Duy Nguyen wrote:
> On Fri, Jul 31, 2015 at 1:06 PM, David Turner <dturner@twopensource.com> wrote:
> > Add a function ref_type, which categorizes refs as per-worktree,
> > pseudoref, or normal ref.
> 
> For per-worktree refs, you probably should follow common_list[] in
> path.c because that's how file-based ref namespace is splitted between
> per-repo and per-worktree, even though just as simple as "everything
> outside refs/ is per-worktree" (with an exception of NOTES_MERGE_REF,
> which should be on the list as well). At least the two should be
> aligned so that the default file-based backend works the same way as
> new backends.

That would be cleaner, I think.  I'm maybe a little worried about
performance if we do this, but I guess we could optimize later. 

Before I re-roll, I'll wait until we come to a conclusion on the 
other per-worktree ref thread.

I think we discussed NOTES_MERGE_REF[1], and decided that it should work
like HEAD.  Does that seem right to you?

> Going further, I think you need to pass the "worktree identifier" to
> ref backend, at least in ref_transaction_begin_fn. Each backend is
> free to store per-worktree refs however it wants. Of course if I ask
> for refs/foo of worktree A, you should not return me refs/foo of
> worktree B. ref_transaction_begin_fn can return a fault code if it
> does not support multiple worktrees, which is fine.

If we did that, we would have to add it all over the place -- not just
ref_transaction_begin, but also update_ref.  

I think it's better to encapsulate this knowledge inside the refs code.

> > Later, we will use this in refs.c to treat pseudorefs specially.
> > Alternate ref backends may use it to treat both pseudorefs and
> > per-worktree refs differently.
> 
> I'm not so sure that this can't be hidden behind backends and they can
> have total control on falling back to file-based, or store them in
> some secondary storage. I haven't re-read your discussion with Junio
> yet (only skimmed through long ago) so I may be missing some important
> points.

The worry is symbolic refs -- a symbolic ref might be a per-worktree ref
pointing to a shared ref pointing to a per-worktree ref.  This is why
it's simpler to let backends handle things.  If we had some rules about
this, we could maybe hide this from the backend, but so far, this was
the simplest thing to do (it works great!).


[1] http://www.spinics.net/lists/git/msg256793.html

Re: [PATCH v5 2/5] refs: add ref_type function

[David Turner]

On Mon, 2015-08-03 at 20:55 +0700, Duy Nguyen wrote:
> On Fri, Jul 31, 2015 at 1:06 PM, David Turner <dturner@twopensource.com> wrote:
> > Add a function ref_type, which categorizes refs as per-worktree,
> > pseudoref, or normal ref.
> 
> For per-worktree refs, you probably should follow common_list[] in
> path.c because that's how file-based ref namespace is splitted between
> per-repo and per-worktree, even though just as simple as "everything
> outside refs/ is per-worktree" (with an exception of NOTES_MERGE_REF,
> which should be on the list as well). At least the two should be
> aligned so that the default file-based backend works the same way as
> new backends.

I've looked into this, and decided not to follow common_list.  That's
because I've hacked the path.c code to treat refs/worktree specially;
it's under refs (common), but it's per-worktree, so it's special-cased.
You may have seen this in the per-worktree-refs-for-bisect thread:
http://permalink.gmane.org/gmane.comp.version-control.git/275673

This will require some special-casing in the alternate backends, but
they can use common the is_per_worktree_ref function.  

[PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions

[David Turner]

Pseudorefs should not be updated through the ref transaction
API, because alternate ref backends still need to store pseudorefs
in GIT_DIR (instead of wherever they store refs).  Instead,
change update_ref and delete_ref to call pseudoref-specific
functions.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 refs.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 93 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 0f87884..e6fc3fe 100644
--- a/refs.c
+++ b/refs.c
@@ -2874,12 +2874,88 @@ enum ref_type ref_type(const char *refname)
        return REF_TYPE_NORMAL;
 }
 
+static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
+			   const unsigned char *old_sha1, struct strbuf *err)
+{
+	const char *filename;
+	int fd;
+	static struct lock_file lock;
+	struct strbuf buf = STRBUF_INIT;
+	int ret = -1;
+
+	strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
+
+	filename = git_path("%s", pseudoref);
+	fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
+	if (fd < 0) {
+		strbuf_addf(err, "Could not open '%s' for writing: %s",
+			    filename, strerror(errno));
+		return -1;
+	}
+
+	if (old_sha1) {
+		unsigned char actual_old_sha1[20];
+		read_ref(pseudoref, actual_old_sha1);
+		if (hashcmp(actual_old_sha1, old_sha1)) {
+			strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
+			rollback_lock_file(&lock);
+			goto done;
+		}
+	}
+
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
+		strbuf_addf(err, "Could not write to '%s'", filename);
+		rollback_lock_file(&lock);
+		goto done;
+	}
+
+	commit_lock_file(&lock);
+	ret = 0;
+done:
+	strbuf_release(&buf);
+	return ret;
+}
+
+static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1)
+{
+	static struct lock_file lock;
+	const char *filename;
+
+	filename = git_path("%s", pseudoref);
+
+	if (old_sha1 && !is_null_sha1(old_sha1)) {
+		int fd;
+		unsigned char actual_old_sha1[20];
+
+		fd = hold_lock_file_for_update(&lock, filename,
+					       LOCK_DIE_ON_ERROR);
+		if (fd < 0)
+			die_errno(_("Could not open '%s' for writing"), filename);
+		read_ref(pseudoref, actual_old_sha1);
+		if (hashcmp(actual_old_sha1, old_sha1)) {
+			warning("Unexpected sha1 when deleting %s", pseudoref);
+			rollback_lock_file(&lock);
+			return -1;
+		}
+
+		unlink(filename);
+		rollback_lock_file(&lock);
+	} else {
+		unlink(filename);
+	}
+
+	return 0;
+}
+
 int delete_ref(const char *refname, const unsigned char *old_sha1,
 	       unsigned int flags)
 {
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 
+	if (ref_type(refname) == REF_TYPE_PSEUDOREF)
+		return delete_pseudoref(refname, old_sha1);
+
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, old_sha1,
@@ -3973,17 +4049,25 @@ int update_ref(const char *msg, const char *refname,
 	       const unsigned char *new_sha1, const unsigned char *old_sha1,
 	       unsigned int flags, enum action_on_err onerr)
 {
-	struct ref_transaction *t;
+	struct ref_transaction *t = NULL;
 	struct strbuf err = STRBUF_INIT;
+	int ret = 0;
 
-	t = ref_transaction_begin(&err);
-	if (!t ||
-	    ref_transaction_update(t, refname, new_sha1, old_sha1,
-				   flags, msg, &err) ||
-	    ref_transaction_commit(t, &err)) {
+	if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+		ret = write_pseudoref(refname, new_sha1, old_sha1, &err);
+	} else {
+		t = ref_transaction_begin(&err);
+		if (!t ||
+		    ref_transaction_update(t, refname, new_sha1, old_sha1,
+					   flags, msg, &err) ||
+		    ref_transaction_commit(t, &err)) {
+			ret = 1;
+			ref_transaction_free(t);
+		}
+	}
+	if (ret) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
-		ref_transaction_free(t);
 		switch (onerr) {
 		case UPDATE_REFS_MSG_ON_ERR:
 			error(str, refname, err.buf);
@@ -3998,7 +4082,8 @@ int update_ref(const char *msg, const char *refname,
 		return 1;
 	}
 	strbuf_release(&err);
-	ref_transaction_free(t);
+	if (t)
+		ref_transaction_free(t);
 	return 0;
 }
 
-- 
2.0.4.315.gad8727a-twtrsrc

Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions

[Stefan Beller]

I am sorry for being late to the review, I looked into coverity today as Duy
bugged me to fix the memory allocation stuff[1]

[1] $gmane/275046

On Thu, Jul 30, 2015 at 11:06 PM, David Turner <dturner@twopensource.com> wrote:

> +
> +       if (old_sha1) {
> +               unsigned char actual_old_sha1[20];
> +               read_ref(pseudoref, actual_old_sha1);

What about the return value of read_ref?
In most cases of the code base (19/21) we check the return of that.
So maybe

    if (read_ref(pseudoref, actual_old_sha1) <0)
        die("Could not read ref %s", pseudoref);


> +               if (fd < 0)
> +                       die_errno(_("Could not open '%s' for writing"), filename);
> +               read_ref(pseudoref, actual_old_sha1);

same here.

Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions

[David Turner]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]


On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
> I am sorry for being late to the review, I looked into coverity today as Duy
> bugged me to fix the memory allocation stuff[1]

Thanks. Junio, can you pleas substitute the attached patch instead?  It
addresses Stefan's issues.  If you prefer, I can resend the whole
series, but I thought this might be easier.

[-- Attachment #2: 0003-pseudorefs-create-and-use-pseudoref-update-and-delet.patch --]
[-- Type: text/x-patch, Size: 4471 bytes --]

From 6c86c38f533c6b35db3a557270aab95b342875c9 Mon Sep 17 00:00:00 2001
From: David Turner <dturner@twopensource.com>
Date: Wed, 15 Jul 2015 18:05:28 -0400
Subject: [PATCH 3/5] pseudorefs: create and use pseudoref update and delete
 functions

Pseudorefs should not be updated through the ref transaction
API, because alternate ref backends still need to store pseudorefs
in GIT_DIR (instead of wherever they store refs).  Instead,
change update_ref and delete_ref to call pseudoref-specific
functions.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 refs.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 95 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 0f87884..e44b88c 100644
--- a/refs.c
+++ b/refs.c
@@ -2874,12 +2874,90 @@ enum ref_type ref_type(const char *refname)
        return REF_TYPE_NORMAL;
 }
 
+static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
+			   const unsigned char *old_sha1, struct strbuf *err)
+{
+	const char *filename;
+	int fd;
+	static struct lock_file lock;
+	struct strbuf buf = STRBUF_INIT;
+	int ret = -1;
+
+	strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
+
+	filename = git_path("%s", pseudoref);
+	fd = hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
+	if (fd < 0) {
+		strbuf_addf(err, "Could not open '%s' for writing: %s",
+			    filename, strerror(errno));
+		return -1;
+	}
+
+	if (old_sha1) {
+		unsigned char actual_old_sha1[20];
+		if (read_ref(pseudoref, actual_old_sha1))
+			die("Could not read ref %s", pseudoref);
+		if (hashcmp(actual_old_sha1, old_sha1)) {
+			strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
+			rollback_lock_file(&lock);
+			goto done;
+		}
+	}
+
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
+		strbuf_addf(err, "Could not write to '%s'", filename);
+		rollback_lock_file(&lock);
+		goto done;
+	}
+
+	commit_lock_file(&lock);
+	ret = 0;
+done:
+	strbuf_release(&buf);
+	return ret;
+}
+
+static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1)
+{
+	static struct lock_file lock;
+	const char *filename;
+
+	filename = git_path("%s", pseudoref);
+
+	if (old_sha1 && !is_null_sha1(old_sha1)) {
+		int fd;
+		unsigned char actual_old_sha1[20];
+
+		fd = hold_lock_file_for_update(&lock, filename,
+					       LOCK_DIE_ON_ERROR);
+		if (fd < 0)
+			die_errno(_("Could not open '%s' for writing"), filename);
+		if (read_ref(pseudoref, actual_old_sha1))
+			die("Could not read ref %s", pseudoref);
+		if (hashcmp(actual_old_sha1, old_sha1)) {
+			warning("Unexpected sha1 when deleting %s", pseudoref);
+			rollback_lock_file(&lock);
+			return -1;
+		}
+
+		unlink(filename);
+		rollback_lock_file(&lock);
+	} else {
+		unlink(filename);
+	}
+
+	return 0;
+}
+
 int delete_ref(const char *refname, const unsigned char *old_sha1,
 	       unsigned int flags)
 {
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 
+	if (ref_type(refname) == REF_TYPE_PSEUDOREF)
+		return delete_pseudoref(refname, old_sha1);
+
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_delete(transaction, refname, old_sha1,
@@ -3973,17 +4051,25 @@ int update_ref(const char *msg, const char *refname,
 	       const unsigned char *new_sha1, const unsigned char *old_sha1,
 	       unsigned int flags, enum action_on_err onerr)
 {
-	struct ref_transaction *t;
+	struct ref_transaction *t = NULL;
 	struct strbuf err = STRBUF_INIT;
+	int ret = 0;
 
-	t = ref_transaction_begin(&err);
-	if (!t ||
-	    ref_transaction_update(t, refname, new_sha1, old_sha1,
-				   flags, msg, &err) ||
-	    ref_transaction_commit(t, &err)) {
+	if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+		ret = write_pseudoref(refname, new_sha1, old_sha1, &err);
+	} else {
+		t = ref_transaction_begin(&err);
+		if (!t ||
+		    ref_transaction_update(t, refname, new_sha1, old_sha1,
+					   flags, msg, &err) ||
+		    ref_transaction_commit(t, &err)) {
+			ret = 1;
+			ref_transaction_free(t);
+		}
+	}
+	if (ret) {
 		const char *str = "update_ref failed for ref '%s': %s";
 
-		ref_transaction_free(t);
 		switch (onerr) {
 		case UPDATE_REFS_MSG_ON_ERR:
 			error(str, refname, err.buf);
@@ -3998,7 +4084,8 @@ int update_ref(const char *msg, const char *refname,
 		return 1;
 	}
 	strbuf_release(&err);
-	ref_transaction_free(t);
+	if (t)
+		ref_transaction_free(t);
 	return 0;
 }
 
-- 
2.4.2.617.g3d4cca8-twtrsrc

Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions

[Junio C Hamano]

David Turner <dturner@twopensource.com> writes:

> On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
>> I am sorry for being late to the review, I looked into coverity today as Duy
>> bugged me to fix the memory allocation stuff[1]
>
> Thanks. Junio, can you pleas substitute the attached patch instead?

No.  The topic is already in 'next', no?

Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> David Turner <dturner@twopensource.com> writes:
>
>> On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
>>> I am sorry for being late to the review, I looked into coverity today as Duy
>>> bugged me to fix the memory allocation stuff[1]
>>
>> Thanks. Junio, can you pleas substitute the attached patch instead?
>
> No.  The topic is already in 'next', no?

Yes, the topic is already in 'next'.  A follow-up fix would be good.

The patch didn't apply cleanly on top of 74ec19d^ to replace 74ec19d
anyway, so I was about to discard it, but after conflict resolution,
the interdiff turns out just these two hunks.

-- >8 --
Subject: pseudoref: check return values from read_ref()
From: David Turner <dturner@twopensource.com>
Date: Wed, 15 Jul 2015 18:05:28 -0400

These codepaths attempt to compare the "expected" current value with
the actual current value, but did not check if we successfully read
the current value before comparison.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * It is likely that we would end up comparing the expected value with
   garbage when the read fails, and the most likely outcome is that
   they do not match and we fail the transaction, which is all fine.

   So in that sense, this is not all that urgent, but it is nice to
   fix it when we know the code is not kosher.

 refs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 522b19b..1db3654 100644
--- a/refs.c
+++ b/refs.c
@@ -2868,7 +2868,9 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
 
 	if (old_sha1) {
 		unsigned char actual_old_sha1[20];
-		read_ref(pseudoref, actual_old_sha1);
+
+		if (read_ref(pseudoref, actual_old_sha1))
+			die("could not read ref '%s'", pseudoref);
 		if (hashcmp(actual_old_sha1, old_sha1)) {
 			strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
 			rollback_lock_file(&lock);
@@ -2904,7 +2906,8 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
 					       LOCK_DIE_ON_ERROR);
 		if (fd < 0)
 			die_errno(_("Could not open '%s' for writing"), filename);
-		read_ref(pseudoref, actual_old_sha1);
+		if (read_ref(pseudoref, actual_old_sha1))
+			die("could not read ref '%s'", pseudoref);
 		if (hashcmp(actual_old_sha1, old_sha1)) {
 			warning("Unexpected sha1 when deleting %s", pseudoref);
 			rollback_lock_file(&lock);

Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions

[David Turner]

On Tue, 2015-08-11 at 15:47 -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > David Turner <dturner@twopensource.com> writes:
> >
> >> On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
> >>> I am sorry for being late to the review, I looked into coverity today as Duy
> >>> bugged me to fix the memory allocation stuff[1]
> >>
> >> Thanks. Junio, can you pleas substitute the attached patch instead?
> >
> > No.  The topic is already in 'next', no?
> 
> Yes, the topic is already in 'next'.  A follow-up fix would be good.
> 
> The patch didn't apply cleanly on top of 74ec19d^ to replace 74ec19d
> anyway, so I was about to discard it, but after conflict resolution,
> the interdiff turns out just these two hunks.
> 
> -- >8 --
> Subject: pseudoref: check return values from read_ref()
> From: David Turner <dturner@twopensource.com>
> Date: Wed, 15 Jul 2015 18:05:28 -0400
> 
> These codepaths attempt to compare the "expected" current value with
> the actual current value, but did not check if we successfully read
> the current value before comparison.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * It is likely that we would end up comparing the expected value with
>    garbage when the read fails, and the most likely outcome is that
>    they do not match and we fail the transaction, which is all fine.
> 
>    So in that sense, this is not all that urgent, but it is nice to
>    fix it when we know the code is not kosher.
> 
>  refs.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 522b19b..1db3654 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2868,7 +2868,9 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
>  
>  	if (old_sha1) {
>  		unsigned char actual_old_sha1[20];
> -		read_ref(pseudoref, actual_old_sha1);
> +
> +		if (read_ref(pseudoref, actual_old_sha1))
> +			die("could not read ref '%s'", pseudoref);
>  		if (hashcmp(actual_old_sha1, old_sha1)) {
>  			strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
>  			rollback_lock_file(&lock);
> @@ -2904,7 +2906,8 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
>  					       LOCK_DIE_ON_ERROR);
>  		if (fd < 0)
>  			die_errno(_("Could not open '%s' for writing"), filename);
> -		read_ref(pseudoref, actual_old_sha1);
> +		if (read_ref(pseudoref, actual_old_sha1))
> +			die("could not read ref '%s'", pseudoref);
>  		if (hashcmp(actual_old_sha1, old_sha1)) {
>  			warning("Unexpected sha1 when deleting %s", pseudoref);
>  			rollback_lock_file(&lock);
> 

LGTM.

[PATCH v5 4/5] bisect: use update_ref

[David Turner]

Instead of manually writing a pseudoref (in one case) and shelling out
to git update-ref (in another), use the update_ref function.  This
is much simpler.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 bisect.c | 37 ++++++++-----------------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/bisect.c b/bisect.c
index 857cf59..33ac88d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -19,7 +19,6 @@ static struct object_id *current_bad_oid;
 
 static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
 static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
-static const char *argv_update_ref[] = {"update-ref", "--no-deref", "BISECT_HEAD", NULL, NULL};
 
 static const char *term_bad;
 static const char *term_good;
@@ -675,34 +674,16 @@ static int is_expected_rev(const struct object_id *oid)
 	return res;
 }
 
-static void mark_expected_rev(char *bisect_rev_hex)
-{
-	int len = strlen(bisect_rev_hex);
-	const char *filename = git_path("BISECT_EXPECTED_REV");
-	int fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
-
-	if (fd < 0)
-		die_errno("could not create file '%s'", filename);
-
-	bisect_rev_hex[len] = '\n';
-	write_or_die(fd, bisect_rev_hex, len + 1);
-	bisect_rev_hex[len] = '\0';
-
-	if (close(fd) < 0)
-		die("closing file %s: %s", filename, strerror(errno));
-}
-
-static int bisect_checkout(char *bisect_rev_hex, int no_checkout)
+static int bisect_checkout(const unsigned char *bisect_rev, int no_checkout)
 {
+	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
-	mark_expected_rev(bisect_rev_hex);
+	memcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), GIT_SHA1_HEXSZ + 1);
+	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 
 	argv_checkout[2] = bisect_rev_hex;
 	if (no_checkout) {
-		argv_update_ref[3] = bisect_rev_hex;
-		if (run_command_v_opt(argv_update_ref, RUN_GIT_CMD))
-			die("update-ref --no-deref HEAD failed on %s",
-			    bisect_rev_hex);
+		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 	} else {
 		int res;
 		res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
@@ -804,7 +785,7 @@ static void check_merge_bases(int no_checkout)
 			handle_skipped_merge_base(mb);
 		} else {
 			printf("Bisecting: a merge base must be tested\n");
-			exit(bisect_checkout(sha1_to_hex(mb), no_checkout));
+			exit(bisect_checkout(mb, no_checkout));
 		}
 	}
 
@@ -948,7 +929,6 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	struct commit_list *tried;
 	int reaches = 0, all = 0, nr, steps;
 	const unsigned char *bisect_rev;
-	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -986,11 +966,10 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	}
 
 	bisect_rev = revs.commits->item->object.sha1;
-	memcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), GIT_SHA1_HEXSZ + 1);
 
 	if (!hashcmp(bisect_rev, current_bad_oid->hash)) {
 		exit_if_skipped_commits(tried, current_bad_oid);
-		printf("%s is the first %s commit\n", bisect_rev_hex,
+		printf("%s is the first %s commit\n", sha1_to_hex(bisect_rev),
 			term_bad);
 		show_diff_tree(prefix, revs.commits->item);
 		/* This means the bisection process succeeded. */
@@ -1003,7 +982,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	       "(roughly %d step%s)\n", nr, (nr == 1 ? "" : "s"),
 	       steps, (steps == 1 ? "" : "s"));
 
-	return bisect_checkout(bisect_rev_hex, no_checkout);
+	return bisect_checkout(bisect_rev, no_checkout);
 }
 
 static inline int log2i(int n)
-- 
2.0.4.315.gad8727a-twtrsrc

[PATCH v5 5/5] sequencer: replace write_cherry_pick_head with update_ref

[David Turner]

Now update_ref (via write_pseudoref) does almost exactly what
write_cherry_pick_head did, so we can remove write_cherry_pick_head
and just use update_ref.

Signed-off-by: David Turner <dturner@twopensource.com>
---
 sequencer.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c4f4b7d..554a704 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -158,23 +158,6 @@ static void free_message(struct commit *commit, struct commit_message *msg)
 	unuse_commit_buffer(commit, msg->message);
 }
 
-static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
-{
-	const char *filename;
-	int fd;
-	struct strbuf buf = STRBUF_INIT;
-
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
-
-	filename = git_path("%s", pseudoref);
-	fd = open(filename, O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"), filename);
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), filename);
-	strbuf_release(&buf);
-}
-
 static void print_advice(int show_hint, struct replay_opts *opts)
 {
 	char *msg = getenv("GIT_CHERRY_PICK_HELP");
@@ -607,9 +590,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * write it at all.
 	 */
 	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
+		update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.sha1, NULL,
+			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
-		write_cherry_pick_head(commit, "REVERT_HEAD");
+		update_ref(NULL, "REVERT_HEAD", commit->object.sha1, NULL,
+			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 
 	if (res) {
 		error(opts->action == REPLAY_REVERT
-- 
2.0.4.315.gad8727a-twtrsrc