threads / patch / 42598

patchfetch: document that pruning happens before fetching

Subject: [PATCH] fetch: document that pruning happens before fetching

## tl;dr

8 messages between Jun 16, 2016 and Jun 16, 2016. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Jeff King· Jun 13, 2016, 23:58 UTC · lore

This was changed in 10a6cc8 (fetch --prune: Run prune before fetching, 2014-01-02), but it seems that nobody in that discussion realized we were advertising the "after" explicitly.

Signed-off-by: Jeff King <peff@peff.net>
---
I include myself in that "nobody" of course. :)
 Documentation/fetch-options.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to Documentation/fetch-options.txt +1 −1
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 036edfb..b05a834 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -52,7 +52,7 @@ ifndef::git-pull[]
 
 -p::
 --prune::
-	After fetching, remove any remote-tracking references that no
+	Before fetching, remove any remote-tracking references that no
 	longer exist on the remote.  Tags are not subject to pruning
 	if they are fetched only because of the default tag
 	auto-following or due to a --tags option.  However, if tags

-- 
2.9.0.150.g8bd4cf6
Jacob Keller· Jun 14, 2016, 06:14 UTC · re: Jeff King · lore

Re: [PATCH] fetch: document that pruning happens before fetching

On Mon, Jun 13, 2016 at 4:58 PM, Jeff King <peff@peff.net> wrote:
Show 25 quoted lines
> This was changed in 10a6cc8 (fetch --prune: Run prune before
> fetching, 2014-01-02), but it seems that nobody in that
> discussion realized we were advertising the "after"
> explicitly.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I include myself in that "nobody" of course. :)
>
>  Documentation/fetch-options.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 036edfb..b05a834 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -52,7 +52,7 @@ ifndef::git-pull[]
>
>  -p::
>  --prune::
> -       After fetching, remove any remote-tracking references that no
> +       Before fetching, remove any remote-tracking references that no
>         longer exist on the remote.  Tags are not subject to pruning
>         if they are fetched only because of the default tag
>         auto-following or due to a --tags option.  However, if tags

What's the difference in behavior due to pruning before instead of after? Curious. It seems like pruning after would make more sense?

Jeff King· Jun 14, 2016, 06:17 UTC · re: Jacob Keller · lore

Re: [PATCH] fetch: document that pruning happens before fetching

On Mon, Jun 13, 2016 at 11:14:36PM -0700, Jacob Keller wrote:
Show 29 quoted lines
> On Mon, Jun 13, 2016 at 4:58 PM, Jeff King <peff@peff.net> wrote:
> > This was changed in 10a6cc8 (fetch --prune: Run prune before
> > fetching, 2014-01-02), but it seems that nobody in that
> > discussion realized we were advertising the "after"
> > explicitly.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > I include myself in that "nobody" of course. :)
> >
> >  Documentation/fetch-options.txt | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> > index 036edfb..b05a834 100644
> > --- a/Documentation/fetch-options.txt
> > +++ b/Documentation/fetch-options.txt
> > @@ -52,7 +52,7 @@ ifndef::git-pull[]
> >
> >  -p::
> >  --prune::
> > -       After fetching, remove any remote-tracking references that no
> > +       Before fetching, remove any remote-tracking references that no
> >         longer exist on the remote.  Tags are not subject to pruning
> >         if they are fetched only because of the default tag
> >         auto-following or due to a --tags option.  However, if tags
> 
> What's the difference in behavior due to pruning before instead of
> after? Curious. It seems like pruning after would make more sense?
See 10a6cc8. :)

Basically, you have to prune first to make way for new incoming refs when there is a D/F conflict.

The downside is that there is a moment where objects may be unreferenced (e.g., if upstream moved "foo" to "bar", we delete "foo" and _then_ create "bar"). And due to the way refs are stored, we do not keep even a reflog for the deleted ref in the interim.

-Peff
Jacob Keller· Jun 14, 2016, 06:18 UTC · re: Jeff King · lore

Re: [PATCH] fetch: document that pruning happens before fetching

On Mon, Jun 13, 2016 at 11:17 PM, Jeff King <peff@peff.net> wrote:
Show 43 quoted lines
> On Mon, Jun 13, 2016 at 11:14:36PM -0700, Jacob Keller wrote:
>
>> On Mon, Jun 13, 2016 at 4:58 PM, Jeff King <peff@peff.net> wrote:
>> > This was changed in 10a6cc8 (fetch --prune: Run prune before
>> > fetching, 2014-01-02), but it seems that nobody in that
>> > discussion realized we were advertising the "after"
>> > explicitly.
>> >
>> > Signed-off-by: Jeff King <peff@peff.net>
>> > ---
>> > I include myself in that "nobody" of course. :)
>> >
>> >  Documentation/fetch-options.txt | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
>> > index 036edfb..b05a834 100644
>> > --- a/Documentation/fetch-options.txt
>> > +++ b/Documentation/fetch-options.txt
>> > @@ -52,7 +52,7 @@ ifndef::git-pull[]
>> >
>> >  -p::
>> >  --prune::
>> > -       After fetching, remove any remote-tracking references that no
>> > +       Before fetching, remove any remote-tracking references that no
>> >         longer exist on the remote.  Tags are not subject to pruning
>> >         if they are fetched only because of the default tag
>> >         auto-following or due to a --tags option.  However, if tags
>>
>> What's the difference in behavior due to pruning before instead of
>> after? Curious. It seems like pruning after would make more sense?
>
> See 10a6cc8. :)
>
> Basically, you have to prune first to make way for new incoming refs
> when there is a D/F conflict.
>
> The downside is that there is a moment where objects may be unreferenced
> (e.g., if upstream moved "foo" to "bar", we delete "foo" and _then_
> create "bar"). And due to the way refs are stored, we do not keep even a
> reflog for the deleted ref in the interim.
>
> -Peff
Ah that makes sense, thanks.

Regards, Jake

Duy Nguyen· Jun 14, 2016, 09:36 UTC · re: Jeff King · lore

Re: [PATCH] fetch: document that pruning happens before fetching

On Tue, Jun 14, 2016 at 6:58 AM, Jeff King <peff@peff.net> wrote:
> This was changed in 10a6cc8 (fetch --prune: Run prune before
> fetching, 2014-01-02), but it seems that nobody in that
> discussion realized we were advertising the "after"
> explicitly.

Ah... ok. Good to know it's moved up top on purpose because I almost tried to move it down :) It's irritating that current output looks like this

<delete ref> <delete ref> <delete ref> remote: <random progress lines> <update ref> <update ref> <update ref>

It probably looks better if we can move the <delete ref> part after "remote: ..." lines (iow still _after_ fetch, but _before_ ref updates), e.g.

remote: <random progress lines> <delete ref> <delete ref> <delete ref> <update ref> <update ref> <update ref>

If we do so, there's no need to update document. But I don't know, maybe it's not worth doing.

Show 26 quoted lines
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I include myself in that "nobody" of course. :)
>
>  Documentation/fetch-options.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 036edfb..b05a834 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -52,7 +52,7 @@ ifndef::git-pull[]
>
>  -p::
>  --prune::
> -       After fetching, remove any remote-tracking references that no
> +       Before fetching, remove any remote-tracking references that no
>         longer exist on the remote.  Tags are not subject to pruning
>         if they are fetched only because of the default tag
>         auto-following or due to a --tags option.  However, if tags
> --
> 2.9.0.150.g8bd4cf6
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Duy
Jeff King· Jun 14, 2016, 10:22 UTC · re: Duy Nguyen · lore

Re: [PATCH] fetch: document that pruning happens before fetching

On Tue, Jun 14, 2016 at 04:36:59PM +0700, Duy Nguyen wrote:
Show 29 quoted lines
> On Tue, Jun 14, 2016 at 6:58 AM, Jeff King <peff@peff.net> wrote:
> > This was changed in 10a6cc8 (fetch --prune: Run prune before
> > fetching, 2014-01-02), but it seems that nobody in that
> > discussion realized we were advertising the "after"
> > explicitly.
> 
> Ah... ok. Good to know it's moved up top on purpose because I almost
> tried to move it down :) It's irritating that current output looks
> like this
> 
> <delete ref>
> <delete ref>
> <delete ref>
> remote: <random progress lines>
> <update ref>
> <update ref>
> <update ref>
> 
> It probably looks better if we can move the <delete ref> part after
> "remote: ..." lines (iow still _after_ fetch, but _before_ ref
> updates), e.g.
> 
> remote: <random progress lines>
> <delete ref>
> <delete ref>
> <delete ref>
> <update ref>
> <update ref>
> <update ref>

I don't think it would be hard to do the deletion separate from the status-printing (in the worst case, you could simply queue the list of deleted refs and then print them later).

That might put the status lines for the deletions farther from any errors or warnings we print when doing the actual deletions, but in theory the error lines are self-contained and can stand on their own.

> If we do so, there's no need to update document. But I don't know,
> maybe it's not worth doing.

I think the documentation should be updated either way. This is not about the ordering in the status table, but rather about the order of the real operations. The user may care about that ordering if they want to know what races are possible, or what would happen if the packfile fetch failed (we'd already have deleted the old refs, but wouldn't fetch the new ones).

-Peff
Duy Nguyen· Jun 14, 2016, 10:36 UTC · re: Jeff King · lore

Re: [PATCH] fetch: document that pruning happens before fetching

On Tue, Jun 14, 2016 at 5:22 PM, Jeff King <peff@peff.net> wrote:
> I think the documentation should be updated either way. This is not
> about the ordering in the status table, but rather about the order of
> the real operations. The user may care about that ordering if they want
> to know what races are possible
Good point.
> or what would happen if the packfile
> fetch failed (we'd already have deleted the old refs, but wouldn't fetch
> the new ones).

Off topic, but this sounds like a bug to me. We could have kept ref update more consistent (maybe at some point we could even do atomic transaction update for all refs if there's a need for it).

-- 
Duy
Jeff King· Jun 15, 2016, 03:46 UTC · re: Duy Nguyen · lore

Re: [PATCH] fetch: document that pruning happens before fetching

On Tue, Jun 14, 2016 at 05:36:35PM +0700, Duy Nguyen wrote:
Show 7 quoted lines
> > or what would happen if the packfile
> > fetch failed (we'd already have deleted the old refs, but wouldn't fetch
> > the new ones).
> 
> Off topic, but this sounds like a bug to me. We could have kept ref
> update more consistent (maybe at some point we could even do atomic
> transaction update for all refs if there's a need for it).

I do think atomic ref transactions are a nice idea, and would probably not be too hard to implement these days (the main thing is just refactoring the deep call stack in git-fetch).

It's possible it could be annoying when you have a single broken ref (and would prefer to just ignore it), but that _should_ be the rare case.

-Peff

← back to recent threads