Santi Béjar [off-list ref] writes:
quoted
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.
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".
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 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?"
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.
2009/6/9 Junio C Hamano [off-list ref]:
Santi Béjar [off-list ref] writes:
quoted
quoted
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
2009/6/9 Santi Béjar [off-list ref]:
2009/6/9 Junio C Hamano [off-list ref]:
quoted
Santi Béjar [off-list ref] writes:
quoted
quoted
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.
At the end it was a little patch to get this corner case working. Here
it is the patch to squash (I'll send later a proper patch mail, with a
test).
And this additional sentence in the commit log:
No behavior changes. The new function behaves like the old code used in
"git pull --rebase".
diff --git i/git-parse-remote.sh w/git-parse-remote.sh
index 8b3ba72..4ac277f 100755
--- i/git-parse-remote.sh
+++ w/git-parse-remote.sh
@@ -238,8 +238,12 @@ get_remote_merge_branch () {
shift
# FIXME: It should return the tracking branch
# Currently only works with the default mapping
- for ref
- do
+ case "$1" in
+ tag)
+ [ -n "$2" ] && echo "refs/tags/${ref}"
+ ;;
+ *)
+ ref=$1
case "$ref" in
+*)
ref=$(expr "z$ref" : 'z+\(.*\)')@@ -251,12 +255,11 @@ get_remote_merge_branch () {
'' | HEAD ) remote=HEAD ;;
heads/*) remote=${remote#heads/} ;;
refs/heads/*) remote=${remote#refs/heads/} ;;
- refs/* | tags/* | remotes/* ) break
+ refs/* | tags/* | remotes/* ) remote=
esac
- echo "refs/remotes/$repo/$remote"
- break
- done
+ [ -n "$remote" ] && echo "refs/remotes/$repo/$remote"
+ esac
;;
esac
}
(Sorry, if it gets corrupted)
Santi
2009/6/9 Santi Béjar [off-list ref]:
2009/6/9 Santi Béjar [off-list ref]:
quoted
2009/6/9 Junio C Hamano [off-list ref]:
quoted
Santi Béjar [off-list ref] writes:
quoted
quoted
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.
At the end it was a little patch to get this corner case working. Here
it is the patch to squash (I'll send later a proper patch mail, with a
test).
And this additional sentence in the commit log:
No behavior changes. The new function behaves like the old code used in
"git pull --rebase".
At the end the original code does not handle this case, as it asumes
that the rebase ref is a remote commit in refs/remotes/$origin/$ref.
So the new code behaves exactly like the old one. I'll send all again
with an updated commit message explaining all this.
Santi