On Thu, Jan 23, 2014 at 11:43 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> 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.
>> #
>> # 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).
>> + 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.
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html