Re: [RFC PATCH 2/1] Make request-pull able to take a refspec of form local:remote
- From
Linus Torvalds <torvalds@linux-foundation.org>
- Date
- Jan 23, 2014, 19:57 UTC
- Message-ID
- <CA+55aFyGaaMOL5pBhZ1BHMr07oDi2MuS-fPu4nnxhjoy+F0AQw@mail.gmail.com>
- In-Reply-To
- <xmqqlhy6trfp.fsf@gitster.dls.corp.google.com>
On Thu, Jan 23, 2014 at 11:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> > I am not sure if it is a good idea to hand-craft "resulting head is > unique" constraint here. We already have disambiguation rules (and > warning mechanism) we use in other places---this part should use the > same rule, I think.
If you can fix that, then yes, that would be lovely. As it is, I couldn't find any easily scriptable way to do that.
Show 15 quoted lines
>> #
>> # Otherwise find a random ref that matches $headrev.
>> find_matching_ref='
>> + my ($head,$headrev) = (@ARGV);
>> + my ($found);
>> +
>> while (<STDIN>) {
>> + chomp;
>> my ($sha1, $ref, $deref) = /^(\S+)\s+([^^]+)(\S*)$/;
>> + my ($pattern);
>> + next unless ($sha1 eq $headrev);
>> +
>> + $pattern="/$head\$";
>
> I think $head is constant inside the loop, so lift it outside?Yes. I'm not really a perl person, and this came from me trying to make the code more readable (and it used to do that magic quoting thing inside the loop, I just used a helper pattern variable).
Show 5 quoted lines
>> + if ($sha1 eq $head) {
>
> I think this is $headrev ($head may be $remote or HEAD), but then
> anything that does not point at $headrev has already been rejected
> at the beginning of this loop, so...?No, this is for when "head" ends up not being a ref, but a SHA1 expression.
IOW, for when you do something odd like
git request-pull HEAD^^ origin HEAD^
when hacking things together. It doesn't actually generate the right request-pull message (because there's no valid branch name), but it *works* in the sense that you can get the diffstat etc and edit things manually.
It's not a big deal - it has never really "worked", and I actually broke that when I then used "$remote" that doesn't actually have the SHA1 any more.
Show 12 quoted lines
>> + if ($found) {
>> print "$found\n";
>> }
>> '
>
> I somehow feel that this is inadequate to catch the "delayed
> propagation" error in the opposite direction. The publish
> repository may have an unrelated ref pointing at the $headrev and we
> may guess that is the ref to be fetched by the integrator based on
> that, but by the time the integrator fetches from the repository,
> the ref may have been updated to its new value that does not match
> $headrev. But I do not think of a way to solve that one.Yes, so you'll get a warning (or, if you get a partial match, maybe not even that), but the important part about all these changes is that it DOESN'T MATTER.
Why? Because it no longer re-writes the target branch name based on that match or non-match. So the pull request will be fine.
In other words, the really fundamental change here i that the "oops, I couldn't find things on the remote" no longer affects the output. It only affects the warning. And I think that's important.
It used to be that the remote matching actually changed the output of the request-pull, and *THAT* was the fundamental problem.
> In any case, shouldn't we be catching duplicate matches here, if the > real objective is to make it less likely for the users to make > mistakes?
It would be good, yes. But my perl-fu is weak, and I really didn't want to worry about it. Also, as above: my primary issue was to not screw up the output, so the remote matching actually has become much less important, and now the warning about it is purely about being helpful, it no longer fundamentally alters any semantics.
So I agree that there is room for improvement, but that's kind of separate from the immediate problem I was trying to solve.
Linus