Differential D21679 Diff 51621 src/applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php

Changeset View

Standalone View

src/applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php

<?php <?php

final class DiffusionMercurialBlameQuery extends DiffusionBlameQuery { final class DiffusionMercurialBlameQuery extends DiffusionBlameQuery {

protected function newBlameFuture(DiffusionRequest $request, $path) { protected function newBlameFuture(DiffusionRequest $request, $path) {

$repository = $request->getRepository(); $repository = $request->getRepository();

$commit = $request->getCommit(); $commit = $request->getCommit();

// NOTE: We're using "--debug" to make "--changeset" give us the full // NOTE: Using "--template" or "--debug" to get the full commit hashes.

// commit hashes. $hg_analyzer = PhutilBinaryAnalyzer::getForBinary('hg');

cspeckmimAuthorUnsubmitted

Done

hg help template doesn't provide any clear distinction of how this works.

This is about all they say about it

Also, for any expression that returns a list, there is a list operator:

  expr % "{template}"

But later in their examples

- Format lists, e.g. files:

    $ hg log -r 0 --template "files:\n{files % '  {file}\n'}"

I'm guessing they're doing some specialized interpretation assuming english language words? Or maybe I lucked out and lines ... line was the right thing to do (as this template worked for me)

cspeckmim: `hg help template` doesn't provide any clear distinction of how this works. This is about all…

epriestleyUnsubmitted

Not Done

I had a lot of difficulty figuring this out, too. I think that in the x % 'y' syntax, {z} means {current item being iterated.z} or something like that, and the existence of line is described in hg help --verbose annotate:

And each entry of "{lines}" provides the following sub-keywords in
addition to "{date}", "{node}", "{rev}", "{user}", etc.

line          String. Line content.
lineno        Integer. Line number at that revision.
path          String. Repository-absolute path of the file at that
              revision.

So I think this is just kind of hard-to-observe, not actually magical? But it took me literally 20 tries to figure out {lines} % '{node}' was the right basic syntax.

epriestley: I had a lot of difficulty figuring this out, too. I think that in the `x % 'y'` syntax, `{z}`…

cspeckmimAuthorUnsubmitted

Done

Aha! I didn't realize that annotate's templating provided more features than the standard hg help template. Also that using --verbose with help commands would provide more help.

cspeckmim: Aha! I didn't realize that annotate's templating provided more features than the standard `hg…

if ($hg_analyzer->isMercurialAnnotateTemplatesAvailable()) {

// See `hg help annotate --verbose` for more info on this template format

$template = "--template {lines % '{node}: {line}'}";

cspeckmimAuthorUnsubmitted

Done

// See `hg help annotate --verbose` for more info on this template format

- $template = "--template {lines % '{node}: {line}'}";

+ $template = "--template \"{lines % '{node}: {line}'}\"";

} else {

I *think* not having the template itself in quotes is causing a problem but this code was working, but then I saw it break just recently when switching back to mercurial 5.8. I think it's fine to update this

cspeckmim: I *think* not having the template itself in quotes is causing a problem but this code was…

} else {

$template = '--debug --changeset';

}

return $repository->getLocalCommandFuture( return $repository->getLocalCommandFuture(

'annotate --debug --changeset --rev %s -- %s', 'annotate %s --rev %s -- %s',

$template,

epriestleyUnsubmitted

Not Done

Although --template probably overrides --changeset, it would be cleaner to move it to the --debug branch, I think (it just affects --debug output).

epriestley: Although `--template` probably overrides `--changeset`, it would be cleaner to move it to the `…

cspeckmimAuthorUnsubmitted

Done

I tried this on a sample file and got the exact same output when --changeset wasn't used with --template, I'll move it

cspeckmim: I tried this on a sample file and got the exact same output when `--changeset` wasn't used with…

$commit, $commit,

$path); $path);

} }

protected function resolveBlameFuture(ExecFuture $future) { protected function resolveBlameFuture(ExecFuture $future) {

list($err, $stdout) = $future->resolve(); list($err, $stdout) = $future->resolve();

if ($err) { if ($err) {

return null; return null;

} }

$result = array(); $result = array();

$lines = phutil_split_lines($stdout); $lines = phutil_split_lines($stdout);

foreach ($lines as $line) { foreach ($lines as $line) {

// If the `--debug` flag was used above instead of `--template` then

// there's a good change additional output was included which is not

// relevant to the information we want. It should be safe to call this

// regardless of whether we used `--debug` or `--template` so there isn't

epriestleyUnsubmitted

Not Done

// relevant to the information we want. It should be safe to call this

- // regardless of whether we used `--debug` or `-template` so there isn't

+ // regardless of whether we used `--debug` or `--template` so there isn't

// a need to track which argument was used.

epriestley:

// a need to track which argument was used.

$line = DiffusionMercurialCommandEngine::filterMercurialDebugOutput(

$line);

epriestleyUnsubmitted

Not Done

I think it would also be fine to just drop this so we can delete the method -- no one has ever noticed issues with annotate that I can recall, and this should converge toward correct behavior as users upgrade -- but up to you.

epriestley: I think it would also be fine to just drop this so we can delete the method -- no one has ever…

cspeckmimAuthorUnsubmitted

Done

I waaaaanna say I've seen some annotations not appear when reviewing certain files in our repository which leads me to think it was likely this issue where debug output caused this to choke. It's probably less likely to be reported since it wasn't displaying an error to the user, and it seems a fair number of stars had to align in our case to hit this issue.

cspeckmim: I //waaaaanna// say I've seen some annotations not appear when reviewing certain files in our…

// Just in case new versions of Mercurial add arbitrary output when using

// the `--debug`, do a quick sanity check that this line is formatted in

// a way we're expecting.

if (strpos(':', $line) === false) {

epriestleyUnsubmitted

Not Done

// a way we're expecting.

- if (!strpos(':', $line)) {

+ if (strpos(':', $line) === false) {

phlog(pht('Unexpected output from hg annotate: %s', $line));

We'll enter this branch if : is not present on the line (strpos() returns false), or if : is present at the beginning of the string (the 0th index, strpos() returns 0). Test against false exactly to be more precise.

epriestley: We'll enter this branch if `:` is not present on the line (strpos() returns false), or if `:`…

cspeckmimAuthorUnsubmitted

Done

Oh, if it's the first character it returns 0 which is then coerced to false? Gotcha.

I did read the PHP documentation on this

Screen Shot 2021-07-08 at 9.45.15 PM.png (598×2 px, 124 KB)

Oh maybe not the big red bit tho. I'll try to read more of those.

cspeckmim: Oh, if it's the first character it returns 0 which is then coerced to false? Gotcha. I did…

phlog(pht('Unexpected output from hg annotate: %s', $line));

continue;

}

list($commit) = explode(':', $line, 2); list($commit) = explode(':', $line, 2);

$result[] = $commit; $result[] = $commit;

} }

return $result; return $result;

} }