Re: [PATCH 4/4] push: add 'prune' option

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 23, 2012, 01:09 UTC
Message-ID: <CAMP44s1s_NeXeUpxDR0cLoLGjSaf-0E_62MzAxeiHS=-6c045A@mail.gmail.com>
In-Reply-To: <7v1upm8jnq.fsf@alter.siamese.dyndns.org>

On Thu, Feb 23, 2012 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> +--prune::
>> +     Remove remote branches that don't have a local counterpart. For example
>> +     a remote branch `tmp` will be removed if a local branch with the same
>> +     name doesn't exist any more. This also respects refspecs, e.g.
>> +     `refs/heads/*:refs/tmp/*` would make sure that remote `refs/tmp/foo`
>> +     will be removed if `refs/heads/foo` doesn't exist.
>
> I do not think it adds much useful information to mention `tmp` only once
> over what is already said by the first sentence.  Also, the first sentence
> of the example does not make it clear that it is assuming a same-for-same
> mapping.

Sure, the first sentence doesn't make it clear, but it would be a valid and obvious assumption. The second sentence makes it clear, and the name `tmp` immediately evokes a branch that will probably be removed.

Show 9 quoted lines

> Coming up with a precise technical description is easy, but it is hard to
> explain to the end user in easy terms, and I commend you for attempting to
> add an example in a short single sentence, though.
>
> Perhaps spelling out the underlying assumption the example makes is the
> best we could do here without going too technical.
>
>        ... For example, if you are pushing all your local branches to
>        update the local branches of the remote,

Yeah, but that's 'git push --all', and that's not the common operation--'git push' is. So that's what I presumed the reader would assume, and it really doesn't make a difference as to what will follow:

>        `tmp` branch will be
>        removed from the remote if you removed your `tmp` branch locally.

This reuses the name `tmp`, which seems to be your objective, but it doesn't explain _why_ it would remove `tmp`; is it because `tmp` is the upstream branch, or is it because it has the same name?

Show 5 quoted lines

>        If you are pushing all your local branches on your laptop to a
>        repository on your desktop machine under `refs/remotes/laptop/`
>        hierarchy to back them up, `refs/remotes/laptop/tmp` is removed
>        from the remote if you no longer have the branch `tmp` on your
>        laptop.

Unfortunately, I as a reader have trouble understanding this. More specifically I have trouble understanding where `refs/remotes/laptop/` is coming from, and what it is meaning. I have always pictured `refs/remotes` as something that 'git fetch' updates, and always from the relevant repository. While eventually I could understand what this thing is doing, it took me more than one read, and I had to read slowly, and even then it seems completely non-standard to me.

I think a synthetic example, like `refs/heads/*:refs/tmp/*`, is much easier to understand because it doesn't mess up with any established refs, and also has the advantage that it shows the relevant refspec, which is useful for people that are not familiar with refspecs, and in fact, people could try it out without messing with their current refs.

Show 15 quoted lines

>> diff --git a/remote.h b/remote.h
>> index b395598..341142c 100644
>> --- a/remote.h
>> +++ b/remote.h
>> @@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);
>>  enum match_refs_flags {
>>       MATCH_REFS_NONE         = 0,
>>       MATCH_REFS_ALL          = (1 << 0),
>> -     MATCH_REFS_MIRROR       = (1 << 1)
>> +     MATCH_REFS_MIRROR       = (1 << 1),
>> +     MATCH_REFS_PRUNE        = (1 << 2),
>>  };
>
> Lose the ',' at the end, for the same reason why deleted line did not have
> one.

And why is that? Isn't git c99? That comma would only ensure that the next patch that touches this would be two lines instead of one.

Cheers.

-- 
Felipe Contreras