Re: [PATCH v8 42/44] refs.c: pass a skip list to name_conflict_fn

2014-05-27 Thread Ronnie Sahlberg 
On Thu, May 22, 2014 at 12:27 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 --- a/refs.c
 +++ b/refs.c
 @@ -798,11 +798,19 @@ struct name_conflict_cb {
   const char *refname;
   const char *oldrefname;
   const char *conflicting_refname;
 + const char **skip;
 + int skipnum;

 Would a struct string_list make sense here?  (See
 Documentation/technical/api-string-list.txt.)

It would. But it is better to do that change later.
Currently we have both repack_without_ref and repack_without_refs that
take the same char ** argument.
After next series we will have removed one of these functions and have
an easier API to modify (once we start tracking the skiplist as part
of the transaction instead).

Instead of doing this change and then redoing it once we move a lot of
the actual work from _commit  to _update
I will do this change towards the end of the next series.


 [...]
  };

  static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
  {
   struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
 + int i;
 + for(i = 0; i  data-skipnum; i++) {


Fixed.

 (style nit) missing space after 'for'.

 + if (!strcmp(entry-name, data-skip[i])) {
 + return 0;
 + }

 Style: git tends to avoid braces around a single-line if/for/etc body.


Fixed.

 [...]
 @@ -817,15 +825,21 @@ static int name_conflict_fn(struct ref_entry *entry, 
 void *cb_data)
   * conflicting with the name of an existing reference in dir.  If
   * oldrefname is non-NULL, ignore potential conflicts with oldrefname
   * (e.g., because oldrefname is scheduled for deletion in the same
 - * operation).
 + * operation). skip contains a list of refs we want to skip checking for
 + * conflicts with. Refs may be skipped due to us knowing that it will
 + * be deleted later during a transaction that deletes one reference and then
 + * creates a new conflicting reference. For example a rename from m to m/m.

 This example of Refs may be skipped due to seems overly complicated.
 Isn't the idea just that skip contains a list of refs scheduled for
 deletion in this transaction, since they shouldn't be treated as
 conflicts at all (for example when renamining m to m/m)?

 I wonder if there's some way to make use of the result of the naive
 refname_available check to decide what to do when creating a ref.

 E.g.: if a refname would be available except there's a ref being
 deleted in the way, we could do one of the following:

  a. delete all relevant loose refs and perform the transaction in
 packed-refs, or

  b. order operations to avoid the D/F conflict, even with loose refs
 (the hardest case is if the ref being deleted uses a directory
 and we want to create a file with the same name.  But that's
 still doable if we're willing to rmdir when needed as part of
 the loop to commit changes)

 The packed-refs trick (a) seems much simpler, but either should work.

 This could be done e.g. by checking is_refname_available with an empty
 list first before doing the real thing with a list of exclusions.

 [...]
 @@ -2592,6 +2609,9 @@ int rename_ref(const char *oldrefname, const char 
 *newrefname, const char *logms
   int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
   const char *symref = NULL;

 + if (!strcmp(oldrefname, newrefname))
 + return 0;

 What is the intended result if I try to rename a nonexistent ref or an
 existent symref to its own name?

Yeah, I should get rid of that.

I have renoved the rename_ref patch and moved it to the next series.
I think we can solve this easier/better once we have the other patches in first.


 Sorry to be so fussy about this part.  It's not that I think that this
 change is trying to do something bad --- in fact, it's more the
 opposite, that I'm excited to see git learning to have a better
 understanding and handling of refname D/F conflicts.

 That would allow:

  * git fetch --prune working as a single transaction even if
the repository being fetched from removed a refs/heads/topic
branch and created refs/heads/topic/1 and refs/heads/topic/2

  * git fast-import and git fetch --mirror learning the same trick

  * fewer code paths having to be touched to be able to (optionally)
let git actually tolerate D/F conflicts, for people who want to
have 'topic', 'topic/1', and 'topic/2' branches at the same time.
This could be turned on by default for remote-tracking refs.  It
would be especially nice for people on Windows and Mac OS where
there can be D/F conflicts that people on Linux didn't notice due
to case-sensitivity.

Longer term, through a configuration that starts turned off by
default and has the default flipped as more people have upgraded
git, this could make D/F conflicts in refnames stop being an error
altogether.

 So it's kind of exciting to see, even though it's fussy to get it
 right.

Re: [PATCH v8 42/44] refs.c: pass a skip list to name_conflict_fn

2014-05-22 Thread Jonathan Nieder 
Ronnie Sahlberg wrote:

 --- a/refs.c
 +++ b/refs.c
 @@ -798,11 +798,19 @@ struct name_conflict_cb {
   const char *refname;
   const char *oldrefname;
   const char *conflicting_refname;
 + const char **skip;
 + int skipnum;

Would a struct string_list make sense here?  (See
Documentation/technical/api-string-list.txt.)

[...]
  };
  
  static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
  {
   struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
 + int i;
 + for(i = 0; i  data-skipnum; i++) {

(style nit) missing space after 'for'.

 + if (!strcmp(entry-name, data-skip[i])) {
 + return 0;
 + }

Style: git tends to avoid braces around a single-line if/for/etc body.

[...]
 @@ -817,15 +825,21 @@ static int name_conflict_fn(struct ref_entry *entry, 
 void *cb_data)
   * conflicting with the name of an existing reference in dir.  If
   * oldrefname is non-NULL, ignore potential conflicts with oldrefname
   * (e.g., because oldrefname is scheduled for deletion in the same
 - * operation).
 + * operation). skip contains a list of refs we want to skip checking for
 + * conflicts with. Refs may be skipped due to us knowing that it will
 + * be deleted later during a transaction that deletes one reference and then
 + * creates a new conflicting reference. For example a rename from m to m/m.

This example of Refs may be skipped due to seems overly complicated.
Isn't the idea just that skip contains a list of refs scheduled for
deletion in this transaction, since they shouldn't be treated as
conflicts at all (for example when renamining m to m/m)?

I wonder if there's some way to make use of the result of the naive
refname_available check to decide what to do when creating a ref.

E.g.: if a refname would be available except there's a ref being
deleted in the way, we could do one of the following:

 a. delete all relevant loose refs and perform the transaction in
packed-refs, or

 b. order operations to avoid the D/F conflict, even with loose refs
(the hardest case is if the ref being deleted uses a directory
and we want to create a file with the same name.  But that's
still doable if we're willing to rmdir when needed as part of
the loop to commit changes)

The packed-refs trick (a) seems much simpler, but either should work.

This could be done e.g. by checking is_refname_available with an empty
list first before doing the real thing with a list of exclusions.

[...]
 @@ -2592,6 +2609,9 @@ int rename_ref(const char *oldrefname, const char 
 *newrefname, const char *logms
   int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
   const char *symref = NULL;
  
 + if (!strcmp(oldrefname, newrefname))
 + return 0;

What is the intended result if I try to rename a nonexistent ref or an
existent symref to its own name?

Sorry to be so fussy about this part.  It's not that I think that this
change is trying to do something bad --- in fact, it's more the
opposite, that I'm excited to see git learning to have a better
understanding and handling of refname D/F conflicts.

That would allow:

 * git fetch --prune working as a single transaction even if
   the repository being fetched from removed a refs/heads/topic
   branch and created refs/heads/topic/1 and refs/heads/topic/2

 * git fast-import and git fetch --mirror learning the same trick

 * fewer code paths having to be touched to be able to (optionally)
   let git actually tolerate D/F conflicts, for people who want to
   have 'topic', 'topic/1', and 'topic/2' branches at the same time.
   This could be turned on by default for remote-tracking refs.  It
   would be especially nice for people on Windows and Mac OS where
   there can be D/F conflicts that people on Linux didn't notice due
   to case-sensitivity.

   Longer term, through a configuration that starts turned off by
   default and has the default flipped as more people have upgraded
   git, this could make D/F conflicts in refnames stop being an error
   altogether.

So it's kind of exciting to see, even though it's fussy to get it
right.

Thanks,
Jonathan
--
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 v8 42/44] refs.c: pass a skip list to name_conflict_fn

2014-05-15 Thread Ronnie Sahlberg 
Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m

For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.

Signed-off-by: Ronnie Sahlberg sahlb...@google.com
---
 refs.c | 43 +--
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index f458ff9..c940509 100644
--- a/refs.c
+++ b/refs.c
@@ -798,11 +798,19 @@ struct name_conflict_cb {
const char *refname;
const char *oldrefname;
const char *conflicting_refname;
+   const char **skip;
+   int skipnum;
 };
 
 static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
 {
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
+   int i;
+   for(i = 0; i  data-skipnum; i++) {
+   if (!strcmp(entry-name, data-skip[i])) {
+   return 0;
+   }
+   }
if (data-oldrefname  !strcmp(data-oldrefname, entry-name))
return 0;
if (names_conflict(data-refname, entry-name)) {
@@ -817,15 +825,21 @@ static int name_conflict_fn(struct ref_entry *entry, void 
*cb_data)
  * conflicting with the name of an existing reference in dir.  If
  * oldrefname is non-NULL, ignore potential conflicts with oldrefname
  * (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with. Refs may be skipped due to us knowing that it will
+ * be deleted later during a transaction that deletes one reference and then
+ * creates a new conflicting reference. For example a rename from m to m/m.
  */
 static int is_refname_available(const char *refname, const char *oldrefname,
-   struct ref_dir *dir)
+   struct ref_dir *dir,
+   const char **skip, int skipnum)
 {
struct name_conflict_cb data;
data.refname = refname;
data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+   data.skip = skip;
+   data.skipnum = skipnum;
 
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) {
@@ -2037,7 +2051,8 @@ int dwim_log(const char *str, int len, unsigned char 
*sha1, char **log)
 
 static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
-   int flags, int *type_p)
+   int flags, int *type_p,
+   const char **skip, int skipnum)
 {
char *ref_file;
const char *orig_refname = refname;
@@ -2084,7 +2099,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char 
*refname,
 * name is a proper prefix of our refname.
 */
if (missing 
-!is_refname_available(refname, NULL, get_packed_refs(ref_cache))) 
{
+!is_refname_available(refname, NULL,
+  get_packed_refs(ref_cache),
+  skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2142,7 +2159,7 @@ struct ref_lock *lock_any_ref_for_update(const char 
*refname,
 const unsigned char *old_sha1,
 int flags, int *type_p)
 {
-   return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+   return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
 }
 
 /*
@@ -2592,6 +2609,9 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
int log = !lstat(git_path(logs/%s, oldrefname), loginfo);
const char *symref = NULL;
 
+   if (!strcmp(oldrefname, newrefname))
+   return 0;
+
if (log  S_ISLNK(loginfo.st_mode))
return error(reflog for %s is a symlink, oldrefname);
 
@@ -2602,10 +2622,12 @@ int rename_ref(const char *oldrefname, const char 
*newrefname, const char *logms
if (!symref)
return error(refname %s not found, oldrefname);
 
-   if (!is_refname_available(newrefname, oldrefname, 
get_packed_refs(ref_cache)))
+   if (!is_refname_available(newrefname, oldrefname,
+ get_packed_refs(ref_cache), NULL, 0))
return 1;
 
-   if (!is_refname_available(newrefname, oldrefname,