Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge

["Santi Béjar" <santi@agolina.net>]

2009/6/9 Junio C Hamano <gitster@pobox.com>:
> Santi Béjar <santi@agolina.net> writes:
>
>>> git pull --rebase tags v1.6.0
>>
>> In fact: git pull --rebase remote tags v1.6.0
>>
>> But this still works because oldremoteref defaults to defaults_merge.
>> So the only behavior change is when a remote branch is
>> rebased/retagged, and you have worst problems then. I think noone used
>> the rebased functionality in this way, so I don't think it is worth to
>> support it. But if someone think it is important I'll do it.
>
> I personally do not think supporting such a form of input is absolutely
> necessary.  Even though technically it might be a regression, if it is so
> rare a form, we can simply say "this strange form used to work, but now it
> does not; you can use this form instead to do the same thing", and move
> on.

OK.

>
> However, at least we should describe the change, both in the commit log
> and documentation.  Simply saying "No behaviour change" is not acceptable,
> when the code clearly is doing something else.  It needs to be backed by
> some explanation, e.g. "Even though this returns different results from
> the original, the caller behaves the same because of such and such
> reasons".

OK.

>
> What caught my attention was not the difference between the new code and
> the original codepath, but your "FIXME" comment that said "Currently only
> works with the default mapping".  My initial reaction was "What?  The new
> code that introduces a function for the specific task of figuring out the
> mapping does not work if the user uses a custom mapping?  What kind of
> improvement is that???".

The original code (in git-pull.sh) behaves like this, I only made it
more explicit.

>
> The reaction was followed by "Even if that were the case, if the original
> code did not work in the case anyway, then it is not a regression.  The
> proposed commit log message claims that there is no behaviour change, so
> that FIXME might not be so grave an offense.  Is it really the case?  Was
> the original broken?"

Yes, the original is broken.

>
> While trying to figure it out, I noticed that the new code does quite a
> different thing (I still haven't figured out the answer to my original
> question about FIXME, by the way).
>
> In any case, if we were changing behaviour by deprecating support for a
> rarely-if-ever used syntax, it would be nice if we at least diagnosed it,
> instead of failing, or worse yet, silently doing something different from
> the old behaviour.

I am not changing the behaviour. The old code worked exactly like the new one.
But I agree that this has to be documented(/deprecated?) somewhere.

The only change in behavior is when a remote tag is rebased/retagged.

Santi