[PATCH] fetch: document that pruning happens before fetching

[PATCH] fetch: document that pruning happens before fetching

[Jeff King]

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
-- 
2.9.0.150.g8bd4cf6

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

[Jacob Keller]

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?

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

[Jeff King]

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

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

[Jacob Keller]

On Mon, Jun 13, 2016 at 11:17 PM, Jeff King <peff@peff.net> wrote:
> 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

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

[Duy Nguyen]

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.

> 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

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

[Jeff King]

On Tue, Jun 14, 2016 at 04:36:59PM +0700, Duy Nguyen wrote:

> 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

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

[Duy Nguyen]

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

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

[Jeff King]

On Tue, Jun 14, 2016 at 05:36:35PM +0700, Duy Nguyen wrote:

> > 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