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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
