On Thu, May 22, 2014 at 12:27 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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.
>
> Thanks,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html