Re: [PATCHv3 1/4] parse-remote: function to get the tracking branch to be merge
- From
- Santi Béjar <santi@agolina.net>
- Date
- Jun 9, 2009, 08:50 UTC
- Message-ID
- <adf1fd3d0906090150k575c538ds28cd8c1a96909e9e@mail.gmail.com>
- In-Reply-To
- <7veittrete.fsf@alter.siamese.dyndns.org>
2009/6/9 Junio C Hamano <gitster@pobox.com>:
Show 17 quoted lines
> 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.
Show 7 quoted lines
> > 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.
Show 7 quoted lines
> > 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.
Show 6 quoted lines
> > 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.
Show 9 quoted lines
> > 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