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

[Jonathan Nieder <jrnieder@gmail.com>]

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