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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Thu, Feb 23, 2012 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

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

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