threads / patch / 35548

patch, 3 partsbuiltin/fetch.c: Add pretty_url() and print_url()

Subject: [PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()

## tl;dr

13 messages between Dec 18, 2013 and Dec 19, 2013. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Tom Miller· Dec 18, 2013, 21:22 UTC · lore

In order to fix branchname DF conflicts during `fetch --prune`, the way the header is output to the screen needs to be refactored. Here is an exmaple of the output with the line in question denoted by '>':

	$ git fetch --prune --dry-run upstream
>	From https://github.com/git/git
	   a155a5f..5512ac5  maint      -> upstream/maint
	   d7aced9..7794a68  master     -> upstream/master
	   523f7c4..3e57c29  next       -> upstream/next
	 + 462f102...0937cdf pu         -> upstream/pu  (forced update)
	   e24105a..5d352bc  todo       -> upstream/todo
	 * [new tag]         v1.8.5.2   -> v1.8.5.2
	 * [new tag]         v1.8.5.2   -> v1.8.5.2

pretty_url(): This function when passed a transport url will anonymize the transport of the url. It will strip a trailing '/'. It will also strip a trailing '.git'. It will return the newly formated url for use. I do not believe there is a need for stripping the trailing '/' and '.git' from a url, but it was already there and I wanted to make as little changes as possible.

print_url(): This function will convert a transport url to a pretty url using pretty_url(). Then it will print out the pretty url to stderr as indicated above in the example output. It uses a global variable named "gshown_url' to prevent this header for being printed twice.

Signed-off-by: Tom Miller <jackerran@gmail.com>
---
 builtin/fetch.c | 60 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 18 deletions(-)
Show changes to builtin/fetch.c +42 −18
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3d978eb..b3145f6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,6 +44,42 @@ static struct transport *gtransport;
 static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
+static int gshown_url = 0;
+
+static char *pretty_url(const char *raw_url) {
+	if (raw_url) {
+		int url_len, i;
+		char *pretty_url, *url;
+
+		url = transport_anonymize_url(raw_url);
+
+		url_len = strlen(url);
+		for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
+			;
+		url_len = i + 1;
+		if (4 < i && !strncmp(".git", url + i - 3, 4))
+			url_len = i - 3;
+
+		pretty_url = xcalloc(1, 1 + url_len);
+		memcpy(pretty_url, url, url_len);
+
+		free(url);
+		return pretty_url;
+	}
+	return xstrdup("foreign");
+}
+
+static void print_url(const char *raw_url) {
+	if (!gshown_url) {
+		char *url = pretty_url(raw_url);
+
+		fprintf(stderr, _("From %s\n"), url);
+
+		gshown_url = 1;
+		free(url);
+	}
+}
+
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -535,7 +571,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 {
 	FILE *fp;
 	struct commit *commit;
-	int url_len, i, shown_url = 0, rc = 0;
+	int url_len, i, rc = 0;
 	struct strbuf note = STRBUF_INIT;
 	const char *what, *kind;
 	struct ref *rm;
@@ -546,10 +582,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	if (!fp)
 		return error(_("cannot open %s: %s\n"), filename, strerror(errno));
 
-	if (raw_url)
-		url = transport_anonymize_url(raw_url);
-	else
-		url = xstrdup("foreign");
+	url = pretty_url(raw_url);
+	url_len = strlen(url);
 
 	rm = ref_map;
 	if (check_everything_connected(iterate_ref_map, 0, &rm)) {
@@ -606,13 +640,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 				what = rm->name;
 			}
 
-			url_len = strlen(url);
-			for (i = url_len - 1; url[i] == '/' && 0 <= i; i--)
-				;
-			url_len = i + 1;
-			if (4 < i && !strncmp(".git", url + i - 3, 4))
-				url_len = i - 3;
-
 			strbuf_reset(&note);
 			if (*what) {
 				if (*kind)
@@ -651,13 +678,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 					    REFCOL_WIDTH,
 					    *what ? what : "HEAD");
 			if (note.len) {
-				if (verbosity >= 0 && !shown_url) {
-					fprintf(stderr, _("From %.*s\n"),
-							url_len, url);
-					shown_url = 1;
-				}
-				if (verbosity >= 0)
+				if (verbosity >= 0) {
+					print_url(raw_url);
 					fprintf(stderr, " %s\n", note.buf);
+				}
 			}
 		}
 	}
-- 
1.8.5.1.163.gd7aced9
Tom Miller· Dec 18, 2013, 21:22 UTC · re: Tom Miller · lore

[PATCH 2/3] fetch --prune: Always print header url

If fetch --prune is run with no new refs to fetch, but it has refs to prune. Then, the header url is not printed as it would if there were new refs to fetch. the following is example output showing this behavior:

$ git fetch --prune --dry-run origin
 x [deleted]         (none)     -> origin/world
After this patch the output of fetch --prune should look like this:
$ git fetch --prune --dry-run origin
From https://github.com/git/git
 x [deleted]         (none)     -> origin/test
Signed-off-by: Tom Miller <jackerran@gmail.com>
---
 builtin/fetch.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Show changes to builtin/fetch.c +6 −3
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b3145f6..e50b697 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -732,7 +732,8 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	return ret;
 }
 
-static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
+static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
+			const char *raw_url)
 {
 	int result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);
@@ -744,6 +745,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
 		if (!dry_run)
 			result |= delete_ref(ref->name, NULL, 0);
 		if (verbosity >= 0) {
+			print_url(raw_url);
 			fprintf(stderr, " x %-*s %-*s -> %s\n",
 				TRANSPORT_SUMMARY(_("[deleted]")),
 				REFCOL_WIDTH, _("(none)"), prettify_refname(ref->name));
@@ -878,11 +880,12 @@ static int do_fetch(struct transport *transport,
 		 * don't care whether --tags was specified.
 		 */
 		if (ref_count) {
-			prune_refs(refs, ref_count, ref_map);
+			prune_refs(refs, ref_count, ref_map, transport->url);
 		} else {
 			prune_refs(transport->remote->fetch,
 				   transport->remote->fetch_refspec_nr,
-				   ref_map);
+				   ref_map,
+				   transport->url);
 		}
 	}
 	free_refs(ref_map);
-- 
1.8.5.1.163.gd7aced9
Tom Miller· Dec 18, 2013, 21:22 UTC · re: Tom Miller · lore

[PATCH 3/3] fetch --prune: Repair branchname DF conflicts

When a branchname DF conflict occurs during a fetch, --prune should be able to fix it. When fetching with --prune, the fetching process happens before pruning causing the branchname DF conflict to persist and report an error. This patch prunes before fetching, thus correcting DF conflicts during a fetch.

Signed-off-by: Tom Miller <jackerran@gmail.com>
Tested-by: Thomas Rast <tr@thomasrast.ch>
---
 builtin/fetch.c  | 10 +++++-----
 t/t5510-fetch.sh | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 5 deletions(-)
Show changes to 2 files +19 −5

builtin/fetch.c, t/t5510-fetch.sh

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e50b697..845c687 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -868,11 +868,6 @@ static int do_fetch(struct transport *transport,
 
 	if (tags == TAGS_DEFAULT && autotags)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
-	if (fetch_refs(transport, ref_map)) {
-		free_refs(ref_map);
-		retcode = 1;
-		goto cleanup;
-	}
 	if (prune) {
 		/*
 		 * We only prune based on refspecs specified
@@ -888,6 +883,11 @@ static int do_fetch(struct transport *transport,
 				   transport->url);
 		}
 	}
+	if (fetch_refs(transport, ref_map)) {
+		free_refs(ref_map);
+		retcode = 1;
+		goto cleanup;
+	}
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 5d4581d..a981125 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -614,4 +614,18 @@ test_expect_success 'all boundary commits are excluded' '
 	test_bundle_object_count .git/objects/pack/pack-${pack##pack	}.pack 3
 '
 
+test_expect_success 'branchname D/F conflict resolved by --prune' '
+	git branch dir/file &&
+	git clone . prune-df-conflict &&
+	git branch -D dir/file &&
+	git branch dir &&
+	(
+		cd prune-df-conflict &&
+		git fetch --prune &&
+		git rev-parse origin/dir >../actual
+	) &&
+	git rev-parse dir >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.1.163.gd7aced9
Junio C Hamano· Dec 18, 2013, 21:54 UTC · re: Tom Miller · lore

Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts

Tom Miller <jackerran@gmail.com> writes:
> When a branchname DF conflict occurs during a fetch,

You may have started with a specific case in which you want to change the behaviour of current Git, so it may be clear what you meant by "branchname DF conflict", but that is true for nobody other than you who will read this log message. Introducing new lingo is OK as long as it is necessary, but in a case like this, where you have to describe what situation you are trying to address anyway, I do not think you need to add a new word to our vocabulary.

	When we have a remote-tracking branch frotz/nitfol from a
	previous fetch, and the upstream now has branch frotz, we
	used to fail to remove frotz/nitfol and recreate frotz with
	"git fetch --prune" from the upstream.
or something like that?

But what should happen when we do not give --prune to "git fetch" in such a situation? Should it fail, because we still have frotz/nitfol and we cannot create frotz without losing it?

Show 8 quoted lines
> --prune should
> be able to fix it. When fetching with --prune, the fetching process
> happens before pruning causing the branchname DF conflict to persist
> and report an error. This patch prunes before fetching, thus
> correcting DF conflicts during a fetch.
>
> Signed-off-by: Tom Miller <jackerran@gmail.com>
> Tested-by: Thomas Rast <tr@thomasrast.ch>

I wasn't following previous threads closely (was there a previous thread???); has this iteration been already tested by trast?

Show 56 quoted lines
> ---
>  builtin/fetch.c  | 10 +++++-----
>  t/t5510-fetch.sh | 14 ++++++++++++++
>  2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index e50b697..845c687 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -868,11 +868,6 @@ static int do_fetch(struct transport *transport,
>  
>  	if (tags == TAGS_DEFAULT && autotags)
>  		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
> -	if (fetch_refs(transport, ref_map)) {
> -		free_refs(ref_map);
> -		retcode = 1;
> -		goto cleanup;
> -	}
>  	if (prune) {
>  		/*
>  		 * We only prune based on refspecs specified
> @@ -888,6 +883,11 @@ static int do_fetch(struct transport *transport,
>  				   transport->url);
>  		}
>  	}
> +	if (fetch_refs(transport, ref_map)) {
> +		free_refs(ref_map);
> +		retcode = 1;
> +		goto cleanup;
> +	}
>  	free_refs(ref_map);
>  
>  	/* if neither --no-tags nor --tags was specified, do automated tag
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 5d4581d..a981125 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -614,4 +614,18 @@ test_expect_success 'all boundary commits are excluded' '
>  	test_bundle_object_count .git/objects/pack/pack-${pack##pack	}.pack 3
>  '
>  
> +test_expect_success 'branchname D/F conflict resolved by --prune' '
> +	git branch dir/file &&
> +	git clone . prune-df-conflict &&
> +	git branch -D dir/file &&
> +	git branch dir &&
> +	(
> +		cd prune-df-conflict &&
> +		git fetch --prune &&
> +		git rev-parse origin/dir >../actual
> +	) &&
> +	git rev-parse dir >expect &&
> +	test_cmp expect actual
> +'
> +
>  test_done
Tom Miller· Dec 19, 2013, 01:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts

On Wed, Dec 18, 2013 at 3:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Tom Miller <jackerran@gmail.com> writes:
>
>> When a branchname DF conflict occurs during a fetch,
>
> You may have started with a specific case in which you want to
> change the behaviour of current Git, so it may be clear what you
> meant by "branchname DF conflict", but that is true for nobody other
> than you who will read this log message.  Introducing new lingo is
> OK as long as it is necessary, but in a case like this, where you
> have to describe what situation you are trying to address anyway,
> I do not think you need to add a new word to our vocabulary.
>
>         When we have a remote-tracking branch frotz/nitfol from a
>         previous fetch, and the upstream now has branch frotz, we
>         used to fail to remove frotz/nitfol and recreate frotz with
>         "git fetch --prune" from the upstream.
>
> or something like that?

I did not intend to introduce new lingo. I did some searching through history to see if something like this had been worked on before and I found a commit by Jeff King that introduced me the the idea of "DF conflicts"

	> commit fa250759794ab98e6edfbbf2f6aa2cb912e535eb
	> Author: Jeff King <peff@peff.net>
	> Date:   Mon May 25 06:40:54 2009 -0400
	>
	>     fetch: report ref storage DF errors more accurately
	>
	>     When we fail to store a fetched ref, we recommend that the
	>     user try running "git prune" to remove up any old refs that
	>     have been deleted by the remote, which would clear up any DF
	>     conflicts. However, ref storage might fail for other
	>     reasons (e.g., permissions problems) in which case the
	>     advice is useless and misleading.
	>
	>     This patch detects when there is an actual DF situation and
	>     only issues the advice when one is found.
	>
	>     Signed-off-by: Jeff King <peff@peff.net>
	>     Signed-off-by: Junio C Hamano <gitster@pobox.com>

I have no issue with rewording the it to be more clear and to try to remove any new lingo.

> But what should happen when we do not give --prune to "git fetch" in
> such a situation?  Should it fail, because we still have frotz/nitfol
> and we cannot create frotz without losing it?

You talk about this to some extent in an email from 2009. I have linked it below for your review. http://article.gmane.org/gmane.comp.version-control.git/132276

In my opinion, if I supply "--prune" to "fetch" I expect it to be destructive. It should be noted that the reflog can *not* be used to recover pruned branches from a remote.

Show 11 quoted lines
>> --prune should
>> be able to fix it. When fetching with --prune, the fetching process
>> happens before pruning causing the branchname DF conflict to persist
>> and report an error. This patch prunes before fetching, thus
>> correcting DF conflicts during a fetch.
>>
>> Signed-off-by: Tom Miller <jackerran@gmail.com>
>> Tested-by: Thomas Rast <tr@thomasrast.ch>
>
> I wasn't following previous threads closely (was there a previous
> thread???); has this iteration been already tested by trast?

There was a previous thread, but I was just looking for feed back on this as a WIP. Should I have replied to it with this patchset?

Here is a link to the previous thread. http://thread.gmane.org/gmane.comp.version-control.git/238530

The commit below should be the same patch he tested. The test was added by him, and I made it part of this commit. Did I do this wrong?

Show 56 quoted lines
>> ---
>>  builtin/fetch.c  | 10 +++++-----
>>  t/t5510-fetch.sh | 14 ++++++++++++++
>>  2 files changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index e50b697..845c687 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -868,11 +868,6 @@ static int do_fetch(struct transport *transport,
>>
>>       if (tags == TAGS_DEFAULT && autotags)
>>               transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
>> -     if (fetch_refs(transport, ref_map)) {
>> -             free_refs(ref_map);
>> -             retcode = 1;
>> -             goto cleanup;
>> -     }
>>       if (prune) {
>>               /*
>>                * We only prune based on refspecs specified
>> @@ -888,6 +883,11 @@ static int do_fetch(struct transport *transport,
>>                                  transport->url);
>>               }
>>       }
>> +     if (fetch_refs(transport, ref_map)) {
>> +             free_refs(ref_map);
>> +             retcode = 1;
>> +             goto cleanup;
>> +     }
>>       free_refs(ref_map);
>>
>>       /* if neither --no-tags nor --tags was specified, do automated tag
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 5d4581d..a981125 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -614,4 +614,18 @@ test_expect_success 'all boundary commits are excluded' '
>>       test_bundle_object_count .git/objects/pack/pack-${pack##pack    }.pack 3
>>  '
>>
>> +test_expect_success 'branchname D/F conflict resolved by --prune' '
>> +     git branch dir/file &&
>> +     git clone . prune-df-conflict &&
>> +     git branch -D dir/file &&
>> +     git branch dir &&
>> +     (
>> +             cd prune-df-conflict &&
>> +             git fetch --prune &&
>> +             git rev-parse origin/dir >../actual
>> +     ) &&
>> +     git rev-parse dir >expect &&
>> +     test_cmp expect actual
>> +'
>> +
>>  test_done
Junio C Hamano· Dec 19, 2013, 06:28 UTC · re: Tom Miller · lore

Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts

Tom Miller <jackerran@gmail.com> writes:
> The commit below should be the same patch he tested. The test was added
> by him, and I made it part of this commit. Did I do this wrong?

No, no, no. All my questions were true questions, not complaints veiled as rhetorical questions. Thanks for many pointers for clarification.

Show 56 quoted lines
>>>> ---
>>>  builtin/fetch.c  | 10 +++++-----
>>>  t/t5510-fetch.sh | 14 ++++++++++++++
>>>  2 files changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>>> index e50b697..845c687 100644
>>> --- a/builtin/fetch.c
>>> +++ b/builtin/fetch.c
>>> @@ -868,11 +868,6 @@ static int do_fetch(struct transport *transport,
>>>
>>>       if (tags == TAGS_DEFAULT && autotags)
>>>               transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
>>> -     if (fetch_refs(transport, ref_map)) {
>>> -             free_refs(ref_map);
>>> -             retcode = 1;
>>> -             goto cleanup;
>>> -     }
>>>       if (prune) {
>>>               /*
>>>                * We only prune based on refspecs specified
>>> @@ -888,6 +883,11 @@ static int do_fetch(struct transport *transport,
>>>                                  transport->url);
>>>               }
>>>       }
>>> +     if (fetch_refs(transport, ref_map)) {
>>> +             free_refs(ref_map);
>>> +             retcode = 1;
>>> +             goto cleanup;
>>> +     }
>>>       free_refs(ref_map);
>>>
>>>       /* if neither --no-tags nor --tags was specified, do automated tag
>>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>>> index 5d4581d..a981125 100755
>>> --- a/t/t5510-fetch.sh
>>> +++ b/t/t5510-fetch.sh
>>> @@ -614,4 +614,18 @@ test_expect_success 'all boundary commits are excluded' '
>>>       test_bundle_object_count .git/objects/pack/pack-${pack##pack    }.pack 3
>>>  '
>>>
>>> +test_expect_success 'branchname D/F conflict resolved by --prune' '
>>> +     git branch dir/file &&
>>> +     git clone . prune-df-conflict &&
>>> +     git branch -D dir/file &&
>>> +     git branch dir &&
>>> +     (
>>> +             cd prune-df-conflict &&
>>> +             git fetch --prune &&
>>> +             git rev-parse origin/dir >../actual
>>> +     ) &&
>>> +     git rev-parse dir >expect &&
>>> +     test_cmp expect actual
>>> +'
>>> +
>>>  test_done
Jeff King· Dec 19, 2013, 11:44 UTC · re: Tom Miller · lore

Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts

On Wed, Dec 18, 2013 at 07:48:59PM -0600, Tom Miller wrote:
> I did not intend to introduce new lingo. I did some searching through
> history to see if something like this had been worked on before and
> I found a commit by Jeff King that introduced me the the idea of
> "DF conflicts"
I take all the blame. :)
As for the patch itself:
Show 25 quoted lines
> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
> >> index e50b697..845c687 100644
> >> --- a/builtin/fetch.c
> >> +++ b/builtin/fetch.c
> >> @@ -868,11 +868,6 @@ static int do_fetch(struct transport *transport,
> >>
> >>       if (tags == TAGS_DEFAULT && autotags)
> >>               transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
> >> -     if (fetch_refs(transport, ref_map)) {
> >> -             free_refs(ref_map);
> >> -             retcode = 1;
> >> -             goto cleanup;
> >> -     }
> >>       if (prune) {
> >>               /*
> >>                * We only prune based on refspecs specified
> >> @@ -888,6 +883,11 @@ static int do_fetch(struct transport *transport,
> >>                                  transport->url);
> >>               }
> >>       }
> >> +     if (fetch_refs(transport, ref_map)) {
> >> +             free_refs(ref_map);
> >> +             retcode = 1;
> >> +             goto cleanup;
> >> +     }

I think this is _probably_ a good thing to do, but it does have an interesting side effect for concurrent operations, and I haven't seen that mentioned so far in the discussion.

Readers of the ref namespace don't have any sort of transactionally consistent view of all of the refs. So if a remote has moved a branch "foo" to "bar" and we "fetch --prune", there will be a moment where a simultaneous reader will see one of two states that never existed on the remote (depending on the order the fetch chooses): either both refs exist, or neither exists.

Right now fetch creates first and deletes after, so a simultaneous reader may see both refs. After your change, it may see no refs at all. Even though both are technically wrong, the current behavior is safer. If the reader is calculating reachability (e.g., for a repack or "git prune), it is better to have too many references than too few.

I'm not sure to what degree we want to care. This is a race, but it's a reasonably unlikely one, and the D/F thing bites people in the real world.

And further confounding this is the fact that even if the writer does everything correctly, the way we read refs can still cause an odd view of the whole namespace. For example, consider moving "refs/heads/z/foo" to "refs/heads/a/foo", while somebody else reads simultaneously. Even with create-before-delete, we can get the sequence:

  1. Reader reads "refs/heads/a/" and sees it does not contain "foo".
  2. Writer writes "refs/heads/a/foo".
  3. Writer deletes "refs/heads/z/foo".
  4. Reader reads "refs/heads/z", which does not contain "foo".

That race can be closed with a double-read of the ref namespaces, but that has poor performance. A more reasonable fix, IMHO, would be to have an alternate ref store that represents transactions atomically (keeping in mind that this really only matters for busy repos with simultaneous readers and writers, so it would not even need to be the default ref store). And once you have such a store, that solves the other problem, too: you can just treat the delete-create as a transaction anyway. It also solves a similar problem with refs that rewind.

So even leaving it as-is does not make the problem go away, though the proposed change does exacerbate it somewhat. I wonder how hard it would be to do the safer thing in the common case that there is no D/F conflict. That is, do multiple passes at updating the refs:

  1. Create/update any refs we can. Those with D/F conflicts are put
     aside for the moment.
  2. Delete any refs according to the --prune rules.
  3. Come back to any D/F conflicts and try them again.

I dunno. As far as I know, this is not a race that people see often in real life (I do not have any confirmed cases of it yet). So it may simply not be worth worrying about.

-Peff
Junio C Hamano· Dec 19, 2013, 19:34 UTC · re: Tom Miller · lore

Re: [PATCH 3/3] fetch --prune: Repair branchname DF conflicts

Tom Miller <jackerran@gmail.com> writes:
Show 7 quoted lines
>> But what should happen when we do not give --prune to "git fetch" in
>> such a situation?  Should it fail, because we still have frotz/nitfol
>> and we cannot create frotz without losing it?
>
> You talk about this to some extent in an email from 2009. I have linked
> it below for your review.
> http://article.gmane.org/gmane.comp.version-control.git/132276

I do not think the old discussion talks about the case. It was about "we have remotes/origin/{frotz,nitfol} from the origin from an earlier fetch, the origin now has updated its frotz and deleted its nitfol. 'git remote prune' removes our remotes/origin/nitfol without updating our copy of remotes/origin/frotz, but I do not think it is sensible. 'git fetch --prune origin' would update both and make our remote-tracking branches for 'origin' in line with the reality". It was not about what 'git fetch' without '--prune' should do.

Your "'git fetch' without '--prune' should be less destrictive" is a good guiding principle. If we have a copy of the 'frotz/nitfol' branch from the 'origin', removing it so that we can have a new copy of the 'frotz' branch the 'origin' now has (after it removed 'frotz/nitfol' to make room) is indeed an operation that loses information. And it probably is the right thing to do to fail such a fetch. 'git fetch --prune' on the other hand really means "I do not care about the branches' histories my 'origin' discarded; bring me up to date and give me the same view as my 'origin' has in my remote-tracking branches", so losing 'frotz/nitfol', which the 'origin' already decided to discard, is what the user wants.

The atomicity issue Peff brings up is an interesting and important one, but I think that is an orthogonal issue.

With the background information from the previous thread between you and trast, the patch [3/3] looks good to me.

Thanks.
Junio C Hamano· Dec 18, 2013, 21:47 UTC · re: Tom Miller · lore

Re: [PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()

Tom Miller <jackerran@gmail.com> writes:
Show 21 quoted lines
> In order to fix branchname DF conflicts during `fetch --prune`, the way
> the header is output to the screen needs to be refactored. Here is an
> exmaple of the output with the line in question denoted by '>':
>
> 	$ git fetch --prune --dry-run upstream
>>	From https://github.com/git/git
> 	   a155a5f..5512ac5  maint      -> upstream/maint
> 	   d7aced9..7794a68  master     -> upstream/master
> 	   523f7c4..3e57c29  next       -> upstream/next
> 	 + 462f102...0937cdf pu         -> upstream/pu  (forced update)
> 	   e24105a..5d352bc  todo       -> upstream/todo
> 	 * [new tag]         v1.8.5.2   -> v1.8.5.2
> 	 * [new tag]         v1.8.5.2   -> v1.8.5.2
>
> pretty_url():
> This function when passed a transport url will anonymize the transport
> of the url. It will strip a trailing '/'. It will also strip a trailing
> '.git'. It will return the newly formated url for use. I do not believe
> there is a need for stripping the trailing '/' and '.git' from a url,
> but it was already there and I wanted to make as little changes as
> possible.

OK. I tend to agree that stripping the trailing part is probably not a good idea and we would want to remove that but that definitely should be done as a separate step, or even as a separate series on top of this one.

Show 5 quoted lines
> print_url():
> This function will convert a transport url to a pretty url using
> pretty_url(). Then it will print out the pretty url to stderr as
> indicated above in the example output. It uses a global variable
> named "gshown_url' to prevent this header for being printed twice.

Gaah. What is that 'g' doing there? Please don't do that meaningless naming.

I do not think the change to introduce such a global variable belongs to this refactoring step. The current caller can decide itself if it called that function, and if you are going to introduce new callers in later steps, they can coordinate among themselves, no?

Tom Miller· Dec 19, 2013, 01:18 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()

On Wed, Dec 18, 2013 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> Tom Miller <jackerran@gmail.com> writes:
>
>> In order to fix branchname DF conflicts during `fetch --prune`, the way
>> the header is output to the screen needs to be refactored. Here is an
>> exmaple of the output with the line in question denoted by '>':
>>
>>       $ git fetch --prune --dry-run upstream
>>>      From https://github.com/git/git
>>          a155a5f..5512ac5  maint      -> upstream/maint
>>          d7aced9..7794a68  master     -> upstream/master
>>          523f7c4..3e57c29  next       -> upstream/next
>>        + 462f102...0937cdf pu         -> upstream/pu  (forced update)
>>          e24105a..5d352bc  todo       -> upstream/todo
>>        * [new tag]         v1.8.5.2   -> v1.8.5.2
>>        * [new tag]         v1.8.5.2   -> v1.8.5.2
>>
>> pretty_url():
>> This function when passed a transport url will anonymize the transport
>> of the url. It will strip a trailing '/'. It will also strip a trailing
>> '.git'. It will return the newly formated url for use. I do not believe
>> there is a need for stripping the trailing '/' and '.git' from a url,
>> but it was already there and I wanted to make as little changes as
>> possible.
>
> OK.  I tend to agree that stripping the trailing part is probably
> not a good idea and we would want to remove that but that definitely
> should be done as a separate step, or even as a separate series on
> top of this one.

I think that removing the trailing part will greatly reduce the complexity to the point were it is unnecessary to have pretty_url(). My goal with extracting this function is to isolate the complexity of formatting the url to a single spot. I am thinking along the lines of the following commit order:

1. Remove the "remove trailing part"
2. Add print_url()
3. Always print url when pruning
4. Reverse order of prune and fetch
Show 8 quoted lines
>> print_url():
>> This function will convert a transport url to a pretty url using
>> pretty_url(). Then it will print out the pretty url to stderr as
>> indicated above in the example output. It uses a global variable
>> named "gshown_url' to prevent this header for being printed twice.
>
> Gaah.  What is that 'g' doing there?  Please don't do that
> meaningless naming.

I am not familiar with C conventions and I was trying to stay consistent. I saw other global variables starting with 'g' and made an assumption. It will use the original name in the upcoming patches.

Show 5 quoted lines
> I do not think the change to introduce such a global variable
> belongs to this refactoring step.  The current caller can decide
> itself if it called that function, and if you are going to introduce
> new callers in later steps, they can coordinate among themselves,
> no?

I agree, there is no reason for introducing it in this step. Thanks for pointing that out.

Thomas Miller· Dec 19, 2013, 17:41 UTC · re: Tom Miller · lore

Re: [PATCH 1/3] builtin/fetch.c: Add pretty_url() and print_url()

On Wed, Dec 18, 2013 at 7:18 PM, Tom Miller <jackerran@gmail.com> wrote:
Show 62 quoted lines
> On Wed, Dec 18, 2013 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Tom Miller <jackerran@gmail.com> writes:
>>
>>> In order to fix branchname DF conflicts during `fetch --prune`, the way
>>> the header is output to the screen needs to be refactored. Here is an
>>> exmaple of the output with the line in question denoted by '>':
>>>
>>>       $ git fetch --prune --dry-run upstream
>>>>      From https://github.com/git/git
>>>          a155a5f..5512ac5  maint      -> upstream/maint
>>>          d7aced9..7794a68  master     -> upstream/master
>>>          523f7c4..3e57c29  next       -> upstream/next
>>>        + 462f102...0937cdf pu         -> upstream/pu  (forced update)
>>>          e24105a..5d352bc  todo       -> upstream/todo
>>>        * [new tag]         v1.8.5.2   -> v1.8.5.2
>>>        * [new tag]         v1.8.5.2   -> v1.8.5.2
>>>
>>> pretty_url():
>>> This function when passed a transport url will anonymize the transport
>>> of the url. It will strip a trailing '/'. It will also strip a trailing
>>> '.git'. It will return the newly formated url for use. I do not believe
>>> there is a need for stripping the trailing '/' and '.git' from a url,
>>> but it was already there and I wanted to make as little changes as
>>> possible.
>>
>> OK.  I tend to agree that stripping the trailing part is probably
>> not a good idea and we would want to remove that but that definitely
>> should be done as a separate step, or even as a separate series on
>> top of this one.
>
> I think that removing the trailing part will greatly reduce the complexity
> to the point were it is unnecessary to have pretty_url().  My goal with
> extracting this function is to isolate the complexity of formatting the
> url to a single spot. I am thinking along the lines of the following
> commit order:
>
> 1. Remove the "remove trailing part"
> 2. Add print_url()
> 3. Always print url when pruning
> 4. Reverse order of prune and fetch
>
>>> print_url():
>>> This function will convert a transport url to a pretty url using
>>> pretty_url(). Then it will print out the pretty url to stderr as
>>> indicated above in the example output. It uses a global variable
>>> named "gshown_url' to prevent this header for being printed twice.
>>
>> Gaah.  What is that 'g' doing there?  Please don't do that
>> meaningless naming.
>
> I am not familiar with C conventions and I was trying to stay consistent.
> I saw other global variables starting with 'g' and made an assumption.
> It will use the original name in the upcoming patches.
>
>> I do not think the change to introduce such a global variable
>> belongs to this refactoring step.  The current caller can decide
>> itself if it called that function, and if you are going to introduce
>> new callers in later steps, they can coordinate among themselves,
>> no?
>
> I agree, there is no reason for introducing it in this step. Thanks for
> pointing that out.

After working on this some more and realizing there is more work to be done on the "fetch --prune should prune before fetching" issue. Also, seeing Jeff's response opened my eyes even more. I believe you are correct. The "trailing parts" piece should be split off into another patch set. I think it would make sense to add the "fetch --prune should print the header url" to that patch set. Should I submit those patches as a separate thread or reply to this thread with just those patches?

-- 
Tom Miller
jackerran@gmail.com
Tom Miller· Dec 19, 2013, 22:57 UTC · re: Tom Miller · lore

[PATCH V2 1/2] fetch --prune: Always print header url

If "fetch --prune" is run with no new refs to fetch, but it has refs to prune. Then, the header url is not printed as it would if there were new refs to fetch.

Output before this patch:
	$ git fetch --prune remote-with-no-new-refs
	 x [deleted]         (none)     -> origin/world
Output after this patch:
	$ git fetch --prune remote-with-no-new-refs
	From https://github.com/git/git.git
	 x [deleted]         (none)     -> origin/test
Signed-off-by: Tom Miller <jackerran@gmail.com>
---
 builtin/fetch.c  | 22 ++++++++++++++++++----
 t/t5510-fetch.sh | 12 ++++++++++++
 2 files changed, 30 insertions(+), 4 deletions(-)
Show changes to 2 files +30 −4

builtin/fetch.c, t/t5510-fetch.sh

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 1e7d617..e6dc2d6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,6 +44,7 @@ static struct transport *gtransport;
 static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
+static int shown_url = 0;
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -535,7 +536,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 {
 	FILE *fp;
 	struct commit *commit;
-	int url_len, i, shown_url = 0, rc = 0;
+	int url_len, i, rc = 0;
 	struct strbuf note = STRBUF_INIT;
 	const char *what, *kind;
 	struct ref *rm;
@@ -708,17 +709,28 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	return ret;
 }
 
-static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
+static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map,
+		const char *raw_url)
 {
 	int result = 0;
 	struct ref *ref, *stale_refs = get_stale_heads(refs, ref_count, ref_map);
+	char *url;
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)")
 		: _("   (%s has become dangling)");
 
+	if (raw_url)
+		url = transport_anonymize_url(raw_url);
+	else
+		url = xstrdup("foreign");
+
 	for (ref = stale_refs; ref; ref = ref->next) {
 		if (!dry_run)
 			result |= delete_ref(ref->name, NULL, 0);
+		if (verbosity >= 0 && !shown_url) {
+			fprintf(stderr, _("From %s\n"), url);
+			shown_url = 1;
+		}
 		if (verbosity >= 0) {
 			fprintf(stderr, " x %-*s %-*s -> %s\n",
 				TRANSPORT_SUMMARY(_("[deleted]")),
@@ -726,6 +738,7 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
 			warn_dangling_symref(stderr, dangling_msg, ref->name);
 		}
 	}
+	free(url);
 	free_refs(stale_refs);
 	return result;
 }
@@ -854,11 +867,12 @@ static int do_fetch(struct transport *transport,
 		 * don't care whether --tags was specified.
 		 */
 		if (ref_count) {
-			prune_refs(refs, ref_count, ref_map);
+			prune_refs(refs, ref_count, ref_map, transport->url);
 		} else {
 			prune_refs(transport->remote->fetch,
 				   transport->remote->fetch_refspec_nr,
-				   ref_map);
+				   ref_map,
+				   transport->url);
 		}
 	}
 	free_refs(ref_map);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 5d4581d..08a4841 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -614,4 +614,16 @@ test_expect_success 'all boundary commits are excluded' '
 	test_bundle_object_count .git/objects/pack/pack-${pack##pack	}.pack 3
 '
 
+test_expect_success 'fetch --prune prints the remotes full url' '
+	git branch goodbye &&
+	git clone . only-prunes &&
+	git branch -D goodbye &&
+	(
+		cd only-prunes &&
+		git fetch --prune origin 2>&1 | head -n1 >../actual
+	) &&
+	echo "From ${D}/." >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.2.194.g00457d4
Tom Miller· Dec 19, 2013, 22:57 UTC · re: Tom Miller · lore

[PATCH V2 2/2] fetch --prune: Run prune before fetching

When we have a remote-tracking branch named "frotz/nitfol" from a previous fetch, and the upstream now has a branch named "frotz". Prior to this patch fetch would fail to remove "frotz/nitfol" with a "git fetch --prune" from the upstream. git would inform the user to use "git remote prune" to fix the problem.

This patch changes the way "fetch --prune" works by moving the pruning operation before the fetching operation. Instead of warning the user of a conflict, it autmatically fixes it.

Signed-off-by: Tom Miller <jackerran@gmail.com>
Tested-by: Thomas Rast <tr@thomasrast.ch>
---
 builtin/fetch.c  | 10 +++++-----
 t/t5510-fetch.sh | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 5 deletions(-)
Show changes to 2 files +19 −5

builtin/fetch.c, t/t5510-fetch.sh

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e6dc2d6..ef3f0bb 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -855,11 +855,6 @@ static int do_fetch(struct transport *transport,
 
 	if (tags == TAGS_DEFAULT && autotags)
 		transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
-	if (fetch_refs(transport, ref_map)) {
-		free_refs(ref_map);
-		retcode = 1;
-		goto cleanup;
-	}
 	if (prune) {
 		/*
 		 * We only prune based on refspecs specified
@@ -875,6 +870,11 @@ static int do_fetch(struct transport *transport,
 				   transport->url);
 		}
 	}
+	if (fetch_refs(transport, ref_map)) {
+		free_refs(ref_map);
+		retcode = 1;
+		goto cleanup;
+	}
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 08a4841..3e64af4 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -626,4 +626,18 @@ test_expect_success 'fetch --prune prints the remotes full url' '
 	test_cmp expect actual
 '
 
+test_expect_success 'branchname D/F conflict resolved by --prune' '
+	git branch dir/file &&
+	git clone . prune-df-conflict &&
+	git branch -D dir/file &&
+	git branch dir &&
+	(
+		cd prune-df-conflict &&
+		git fetch --prune &&
+		git rev-parse origin/dir >../actual
+	) &&
+	git rev-parse dir >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.5.2.194.g00457d4

← back to recent threads