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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Feb 23, 2012, 02:30 UTC
Message-ID: <CAMP44s1i6LaXgfbMfx+hhE20NbaBZ+npAZXNSUb79nRkKzpVWg@mail.gmail.com>
In-Reply-To: <7vlinu72ui.fsf@alter.siamese.dyndns.org>

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

Show 9 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Feb 23, 2012 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Yeah, but that's 'git push --all', and that's not the common
>> operation--'git push' is.
>
> "git push" is common but that does not give you a solid base to guess what
> the reader's assumption would be.  Are you assuming "matching" semantics?

But if you want to spell the assumption, why not spell the most common one?

Show 6 quoted lines

>> So that's what I presumed the reader would
>> assume,...
>
> I do not want let us guess what the reader assumes, as many people seem to
> suggest setting push.default to different values and that would change
> what the reader would assume.

No, it wouldn't. Almost all readers are familiar about what 'git push' does by default--I would even adventure to say all--the ones that set a custom push.default would do it because they _know_ they don't want the default.

And in fact, as I said, it doesn't matter what the reader has set in push.default, or if the reader is assuming 'git push --all', or 'git push :', or 'git push --tags', or 'git push refs/heads/*', the end result is the same.

> That was the whole reason that I suggested
> to spell the assumption out, so that the reader's assumption does not have
> to get into the picture.

I don't think it matters what the reader assumes, because the end result is the same, so there's no reason to spell any assumption.

Show 8 quoted lines

>> 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?
>
> The example is to clarify "local counterpart" in the main text.  I
> actually would prefer to get rid of `tmp` but I left it as-is as you
> wrote.  The exact name used in the example does not matter, whether it is
> `tmp` or `xyzzy`.

I believe `tmp` is more useful than `xyzzy`, or even more than `master`. I already explained why.

Show 9 quoted lines

>> 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.
>
> The layout is the recommended set-up to emulate a fetch with a push in the
> reverse direction, which I thought anybody should notice.  It is a failure
> in our documentation that even an expert didn't.

The problem is not figuring out the `refs/remotes/foo`, but picturing the fact that there are two repositories, the user ran on one of them 'git remote add laptop', and then 'git remote add pc', and then 'git push --prune refs/heads*:refs/remotes/laptop/*'. Lots of things to keep in mind for this paragraph. And, as you say, it's the reverse direction of a fetch, so it's immediately weird.

Moreover, I find it strange that you want to rely on the assumption that the reader is familiar with the pattern `refs/remotes/foo`, and the refspec syntax, which is pretty deep plumbing, and not on the default action of 'git push', which is high-level porcelain, and as I said, not even needed to understand my example.

Show 19 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?
>
> Because I told you so ;-).

You didn't tell me anything about the previous line :)

> More seriously, we have had patches to accomodate other people's compilers
> by dropping the last comma in enum {}.  See c9b6782 (enums: omit trailing
> comma for portability, 2011-03-16), 4b05548 (enums: omit trailing comma
> for portability, 2010-05-14) for examples.

Yeah, I remembered those patches after sending the mail. Shame. Maybe next decade =/

Cheers.

-- 
Felipe Contreras