Re: [PATCH v3 05/44] refs.c: move update_ref to refs.c

[David Turner]

On Tue, 2015-10-13 at 05:41 +0200, Michael Haggerty wrote:

> If its removal was intentional, it deserves a careful explanation (and
> should probably be done as a separate commit). If it was an accident,
> please consider how this accident arose and try to think about whether
> similar accidents might have happened elsewhere in this series.

This was an accident. I think it must have happened when I
forward-ported Ronnie's changes over my change that introduced that
check.  Usually, when there were conflicts during this process
(indicating that the moved code had changed in the meantime), I did the
move by copy-pasting the code (rather than by choosing the old version).
Apparently, I missed this one.

Will fix.

> > [...]
> 
> Michael
> 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 05/44] refs.c: move update_ref to refs.c

[David Turner]

On Tue, 2015-10-13 at 05:41 +0200, Michael Haggerty wrote:

> The original read
> 
>   if (read_ref(pseudoref, actual_old_sha1))
>   die("could not read ref '%s'", pseudoref);
> 
> This seems like an important test. What happened to it?
> 
> If its removal was intentional, it deserves a careful explanation (and
> should probably be done as a separate commit). If it was an accident,
> please consider how this accident arose and try to think about whether
> similar accidents might have happened elsewhere in this series.

I went ahead and manually rechecked all of the patches to ensure that
nothing else was screwed up.  While doing so, I found two or three
spurious whitespace changes (moving newlines around), which I fixed (in
refs-backend-pre-vtable, which I'll send to the list tomorrow-ish).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 05/44] refs.c: move update_ref to refs.c

[David Turner]

From: Ronnie Sahlberg 

Move update_ref() to the refs.c file since this function does not
contain any backend specific code.  Move the ref classifier functions
as well, since update_ref depends on them.

Based on Ronnie Sahlberg's patch

Signed-off-by: Ronnie Sahlberg 
Signed-off-by: David Turner 
---
 refs-be-files.c | 117 +---
 refs.c  | 115 +++
 2 files changed, 116 insertions(+), 116 deletions(-)

diff --git a/refs-be-files.c b/refs-be-files.c
index d0dfdfc..7fe4931 100644
--- a/refs-be-files.c
+++ b/refs-be-files.c
@@ -2675,8 +2675,6 @@ struct pack_refs_cb_data {
struct ref_to_prune *ref_to_prune;
 };
 
-static int is_per_worktree_ref(const char *refname);
-
 /*
  * An each_ref_entry_fn that is run over loose references only.  If
  * the loose reference can be packed, add an entry in the packed ref
@@ -2691,7 +2689,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, 
void *cb_data)
int is_tag_ref = starts_with(entry->name, "refs/tags/");
 
/* Do not pack per-worktree refs: */
-   if (is_per_worktree_ref(entry->name))
+   if (ref_type(entry->name) == REF_TYPE_PER_WORKTREE)
return 0;
 
/* ALWAYS pack tags */
@@ -2879,77 +2877,6 @@ 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") ||
-   starts_with(refname, "refs/bisect/");
-}
-
-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;
-}
-
-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(, "%s\n", sha1_to_hex(sha1));
-
-   filename = git_path("%s", pseudoref);
-   fd = hold_lock_file_for_update(, 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();
-   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();
-   goto done;
-   }
-
-   commit_lock_file();
-   ret = 0;
-done:
-   strbuf_release();
-   return ret;
-}
-
 static int delete_pseudoref(const char *pseudoref, const unsigned char 
*old_sha1)
 {
static struct lock_file lock;
@@ -4098,48 +4025,6 @@ int ref_transaction_verify(struct ref_transaction 
*transaction,
  flags, NULL, err);
 }
 
-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 = NULL;
-   struct strbuf err = STRBUF_INIT;
-   int ret = 0;
-
-   if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
-   ret = write_pseudoref(refname, new_sha1, old_sha1, );
-   } else {
-   t = ref_transaction_begin();
-   if (!t ||
-   ref_transaction_update(t, refname, new_sha1, old_sha1,
-  flags, msg, ) ||
-   ref_transaction_commit(t, )) {
-   ret = 1;
-   ref_transaction_free(t);
-   }
-   }
-   if (ret) {
-   const char *str = "update_ref failed for ref '%s': %s";
-
-   switch (onerr) {
-   case UPDATE_REFS_MSG_ON_ERR:
-   error(str, refname, err.buf);
-   break;
-   case UPDATE_REFS_DIE_ON_ERR:
-

Re: [PATCH v3 05/44] refs.c: move update_ref to refs.c

[Michael Haggerty]

On 10/12/2015 11:51 PM, David Turner wrote:
> From: Ronnie Sahlberg 
> 
> Move update_ref() to the refs.c file since this function does not
> contain any backend specific code.  Move the ref classifier functions
> as well, since update_ref depends on them.
> 
> Based on Ronnie Sahlberg's patch
> 
> Signed-off-by: Ronnie Sahlberg 
> Signed-off-by: David Turner 
> ---
>  refs-be-files.c | 117 
> +---
>  refs.c  | 115 +++
>  2 files changed, 116 insertions(+), 116 deletions(-)
> 
> diff --git a/refs-be-files.c b/refs-be-files.c
> index d0dfdfc..7fe4931 100644
> --- a/refs-be-files.c
> +++ b/refs-be-files.c
> [...]
> @@ -2879,77 +2877,6 @@ static int delete_ref_loose(struct ref_lock *lock, int 
> flag, struct strbuf *err)
> [...]
> -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(, "%s\n", sha1_to_hex(sha1));
> -
> - filename = git_path("%s", pseudoref);
> - fd = hold_lock_file_for_update(, 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();
> - 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();
> - goto done;
> - }
> -
> - commit_lock_file();
> - ret = 0;
> -done:
> - strbuf_release();
> - return ret;
> -}
> -
> [...]
> diff --git a/refs.c b/refs.c
> index 77492ff..2d10708 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1,3 +1,118 @@
> [...]
> +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(, "%s\n", sha1_to_hex(sha1));
> +
> + filename = git_path("%s", pseudoref);
> + fd = hold_lock_file_for_update(, 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);

The original read

if (read_ref(pseudoref, actual_old_sha1))
die("could not read ref '%s'", pseudoref);

This seems like an important test. What happened to it?

If its removal was intentional, it deserves a careful explanation (and
should probably be done as a separate commit). If it was an accident,
please consider how this accident arose and try to think about whether
similar accidents might have happened elsewhere in this series.

> [...]

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html