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

On Thu, May 22, 2014 at 12:27 PM, Jonathan Nieder [off-list ref] wrote:

Ronnie Sahlberg wrote:

quoted

--- 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.

[...]

quoted

 };

 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'.

quoted

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

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

Fixed.

[...]

quoted

@@ -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.

[...]

quoted

@@ -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.

Thanks,
Jonathan