Re: [PATCH v3 02/44] refs: make repack_without_refs and is_branch public

[Michael Haggerty]

On 10/13/2015 04:39 AM, Michael Haggerty wrote:
> On 10/12/2015 11:51 PM, David Turner wrote:
>> is_branch was already non-static, but this patch declares it in the
>> header.
>>
>> Signed-off-by: Ronnie Sahlberg 
>> Signed-off-by: David Turner 
>> ---
>> [...]
> 
> It seems odd that repack_without_refs() should be made public (and
> ultimately end up in refs.h) given that it intrinsically only has to do
> with file-based references. But I will read on...

I think the reason you needed to do this was because you wanted to move
delete_refs() to the common code. It is true that delete_ref() can be
moved to the common code. And most of the code in delete_refs() is just
a loop calling delete_ref(). But delete_refs() also does the very
files-specific optimization of calling repack_without_refs() before the
loop. *That* call shouldn't be in the common code.

So my suggestion is that you write a common_delete_refs() function that
only includes the loop over delete_ref(), and a files_delete_refs()
function that is basically

{
result = repack_without_refs(refnames, );
if (result) {
...report error...
return result;
}
return common_delete_refs(...);
}

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

Re: [PATCH v3 02/44] refs: make repack_without_refs and is_branch public

[David Turner]

On Tue, 2015-10-13 at 09:23 +0200, Michael Haggerty wrote:
> On 10/13/2015 04:39 AM, Michael Haggerty wrote:
> > On 10/12/2015 11:51 PM, David Turner wrote:
> >> is_branch was already non-static, but this patch declares it in the
> >> header.
> >>
> >> Signed-off-by: Ronnie Sahlberg 
> >> Signed-off-by: David Turner 
> >> ---
> >> [...]
> > 
> > It seems odd that repack_without_refs() should be made public (and
> > ultimately end up in refs.h) given that it intrinsically only has to do
> > with file-based references. But I will read on...
> 
> I think the reason you needed to do this was because you wanted to move
> delete_refs() to the common code. It is true that delete_ref() can be
> moved to the common code. And most of the code in delete_refs() is just
> a loop calling delete_ref(). But delete_refs() also does the very
> files-specific optimization of calling repack_without_refs() before the
> loop. *That* call shouldn't be in the common code.
> 
> So my suggestion is that you write a common_delete_refs() function that
> only includes the loop over delete_ref(), and a files_delete_refs()
> function that is basically
> 
> {
> result = repack_without_refs(refnames, );
> if (result) {
> ...report error...
> return result;
> }
> return common_delete_refs(...);
> }

OK, I can do that.  That will have to be part of the backends work, so
I'll exclude it from the refs-backend-pre-vtable patch set.

--
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 02/44] refs: make repack_without_refs and is_branch public

[David Turner]

is_branch was already non-static, but this patch declares it in the
header.

Signed-off-by: Ronnie Sahlberg 
Signed-off-by: David Turner 
---
 refs.c |  9 +
 refs.h | 13 +
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index fe71ea0..d0dfdfc 100644
--- a/refs.c
+++ b/refs.c
@@ -2814,14 +2814,7 @@ int pack_refs(unsigned int flags)
return 0;
 }
 
-/*
- * Rewrite the packed-refs file, omitting any refs listed in
- * 'refnames'. On error, leave packed-refs unchanged, write an error
- * message to 'err', and return a nonzero value.
- *
- * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
- */
-static int repack_without_refs(struct string_list *refnames, struct strbuf 
*err)
+int repack_without_refs(struct string_list *refnames, struct strbuf *err)
 {
struct ref_dir *packed;
struct string_list_item *refname;
diff --git a/refs.h b/refs.h
index 79ea220..729bc3c 100644
--- a/refs.h
+++ b/refs.h
@@ -218,6 +218,19 @@ extern void warn_dangling_symrefs(FILE *fp, const char 
*msg_fmt, const struct st
 int pack_refs(unsigned int flags);
 
 /*
+ * Rewrite the packed-refs file, omitting any refs listed in
+ * 'refnames'. On error, packed-refs will be unchanged, the return
+ * value is nonzero, and a message about the error is written to the
+ * 'err' strbuf.
+ *
+ * The refs in 'refnames' needn't be sorted. `err` must not be NULL.
+ */
+extern int repack_without_refs(struct string_list *refnames,
+  struct strbuf *err);
+
+extern int is_branch(const char *refname);
+
+/*
  * Return true iff a reference named refname could be created without
  * conflicting with the name of an existing reference.  If
  * skip is non-NULL, ignore potential conflicts with refs in skip
-- 
2.4.2.644.g97b850b-twtrsrc

--
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 02/44] refs: make repack_without_refs and is_branch public

[Michael Haggerty]

On 10/12/2015 11:51 PM, David Turner wrote:
> is_branch was already non-static, but this patch declares it in the
> header.
> 
> Signed-off-by: Ronnie Sahlberg 
> Signed-off-by: David Turner 
> ---
> [...]

It seems odd that repack_without_refs() should be made public (and
ultimately end up in refs.h) given that it intrinsically only has to do
with file-based references. But I will read on...

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