fix Issue 15272 - [2.069-rc2,inline] nothing written to output when -… #5253

WalterBright · 2015-11-02T23:38:00Z

…inline is set

https://issues.dlang.org/show_bug.cgi?id=15272

Not sure at the moment how to create a test case for it.

MartinNowak · 2015-11-02T23:59:54Z

Regression fixes always against the stable branch please.

MartinNowak · 2015-11-03T00:00:28Z

src/backend/cgelem.c

-                e2->E1 = eb;
-                e2->E2 = e;
+                e2->E1 = e;
+                e2->E2 = eb;


How does switching the assignment order help?

So the two leaves are executed in the same order as in the OPpair expression node.

WalterBright · 2015-11-03T00:00:41Z

Evidently, this is not the answer, either:

core.exception.AssertError@std/encoding.d(269): unittest failure

WalterBright · 2015-11-03T02:51:51Z

Hah! Turns out that safety0ff.bugz was on the right track, but there was yet another bug that needed to be fixed to get all this to work.

WalterBright · 2015-11-03T03:23:29Z

Added test cases.

…inline is set

WalterBright · 2015-11-03T03:44:03Z

I've tried setting it against another branch, and have failed. Besides, I still don't understand what's wrong with cherry-pick. I use it all the time to put improvements into the D1 branch. And in any case, PRs on the release branch have to be cherry-picked back into HEAD anyway. So I simply don't understand the problem.

aG0aep6G · 2015-11-03T06:47:32Z

test15272 passes without the fix, too.

WalterBright · 2015-11-03T07:42:14Z

The test failing relies on the contents of an uninitialized variable, hence the weird constructs in the bugzilla report. There's not really a reliable way to make it fail - however, test15272 should always pass.

CyberShadow · 2015-11-03T07:50:56Z

I've tried setting it against another branch,

When you start making the changes, instead of starting from the latest master start from the latest stable.
When you create the PR on GitHub, there will be a branch dropdown. Just choose stable. You should then see only your own commits in the list.

and have failed.

This non-specific complaining isn't helping anyone.

Besides, I still don't understand what's wrong with cherry-pick.

Many things. This has been discussed many, many times. There is no need to pretend this is a new topic or problem.

WalterBright · 2015-11-03T09:02:02Z

What was never answered is why merging commits from stable to master is good but merging commits from master to stable is bad. What's the difference?

CyberShadow · 2015-11-03T09:06:48Z

What was never answered is why merging commits from stable to master is good but merging commits from master to stable is bad. What's the difference?

Merging is not the same as cherry-picking. The question to ask here is what the advantage of merging is over cherry-picking. With cherry-picking, the commit is duplicated (essentially copied and pasted) into a new branch, which causes two distinct commits with the same set of changes to exist in D's history. This causes confusion and problems for tools such as Digger.

We can't merge master into stable because aside from bringing in the regression fix, it would also pull in all unstable commits made on the master branch that don't belong in stable. As such, the only options are to either cherry-pick (which has the problems mentioned above) or to make the pull request against the stable branch in the first place.

Hope this helps! Happy to clarify if something else is not clear.

WalterBright · 2015-11-03T09:40:50Z

Thanks for the explanation. So what happens if a fix is made to stable only, and is not meant for master?

Also, why not fix Digger?

WalterBright · 2015-11-03T09:42:31Z

When you start making the changes, instead of starting from the latest master start from the latest stable .

mercury ~/forks/dmd> git checkout stable
error: pathspec 'stable' did not match any file(s) known to git.

I thought my git fork contained everything.

CyberShadow · 2015-11-03T09:46:19Z

Thanks for the explanation. So what happens if a fix is made to stable only, and is not meant for master?

It can be undone as part of the stable-to-master merge PR. But has such a situation arisen before?

Also, why not fix Digger?

It is a complicated problem. Digger would need to search D's history for similar commits, and it would need to make assumptions about the structure of the repository's history. But this isn't the only issue caused by cherry-picking.

I thought my git fork contained everything.

That's strange. Do you see origin/stable when you run git branch -r?

WalterBright · 2015-11-03T09:59:12Z

But has such a situation arisen before?

It will.

WalterBright · 2015-11-03T10:01:23Z

Do you see origin/stable when you run git branch -r ?

I see:

  ...
  origin/simplify-strings
  origin/size_t_error
  origin/speed_up_search
  origin/splitpair
  origin/staging
  origin/statement-errors
  origin/std-mangle2
  origin/std-mangling
  origin/strictalias
  origin/stringbloat
  origin/stringexp
  origin/stripParams
  origin/sync
  ...

CyberShadow · 2015-11-03T10:08:06Z

Ah, origin is your fork. Okay. Do you know the remote name for the D-Programming-Language/dmd repo? It'll be in the output of git remote -v.

WalterBright · 2015-11-03T10:12:50Z

mercury ~/forks/dmd> git remote -v
origin  git@github.com:WalterBright/dmd.git (fetch)
origin  git@github.com:WalterBright/dmd.git (push)
upstream        git@github.com:D-Programming-Language/dmd (fetch)
upstream        git@github.com:D-Programming-Language/dmd (push)

CyberShadow · 2015-11-03T10:22:29Z

OK, this will set up the stable branch for you:

git fetch upstream
git branch --track stable upstream/stable
git checkout stable

WalterBright · 2015-11-03T10:32:44Z

2.When you create the PR on GitHub, there will be a branch dropdown. Just choose stable .

Stable is not in the dropdown list.

CyberShadow · 2015-11-03T10:37:28Z

Sorry, I should've been more specific, forgot there's like 4 dropdowns.

This one:

WalterBright · 2015-11-03T10:53:18Z

There is no "base fork:" nor a "head fork:" or "base: master" on mine. There's only "Branch: master".

CyberShadow · 2015-11-03T10:58:41Z

Oh, that's on a different page.

Do you see a button like this on https://github.com/WalterBright/dmd ?

If so, click the green button and you'll get to the page in the above screenshots. If not, select your pull request's branch (fix15272b I guess) and click the button.

(BTW, this is the same as how one would normally do a PR against master. I guess you've been using some other way?)

MartinNowak · 2015-11-03T13:16:33Z

main reasons to avoid cherry-picking:

copy&pasting commits creates lots of conflicts when merging stable into master
makes it very hard to track which fix is in which branch, and subsequently we "forgot" a number of fixes in the past releases
ultimately the person creating a PR has to suggest which branch to target, everybody has to
get involved at least a bit in making the next release to scale our development process

Thanks for the explanation. So what happens if a fix is made to stable only, and is not meant for master?

We'd still merge it into master and revert/change the fix then.

See #5258 for the rebased PR.

WalterBright · 2015-11-04T02:21:24Z

The trouble seems to be that my fork does not contain 'stable'. I do not know how to make it do that.

yebblies · 2015-11-04T02:26:03Z

git fetch upstream
git push origin stable

CyberShadow · 2015-11-04T08:21:40Z

Your fork doesn't need to have a stable branch. Why do you think that your fork must have a stable branch?

WalterBright · 2015-11-04T23:09:51Z

Because github wouldn't list stable as something I could put a PR against. As described upthread.

WalterBright · 2015-11-04T23:10:41Z

Thanks, @yebblies

CyberShadow · 2015-11-04T23:11:01Z

I think you're looking in the wrong place. Look at the screenshots I posted.

Can you post a screenshot yourself?

CyberShadow · 2015-11-04T23:13:48Z

My dmd fork doesn't have a stable branch, but that doesn't prevent me from creating PRs that target stable. I'm pretty sure you're on the wrong track here. @yebblies Can you explain why you posted those commands?

yebblies · 2015-11-05T02:40:29Z

@yebblies Can you explain why you posted those commands?

Alien mind control.

CyberShadow · 2015-11-05T02:56:19Z

Oh, OK. Carry on then :)

On Thu, Nov 5, 2015 at 4:40 AM, Daniel Murphy notifications@github.com
wrote:

@yebblies https://github.com/yebblies Can you explain why you posted
those commands?

Alien mind control.

—
Reply to this email directly or view it on GitHub
#5253 (comment)
.

WalterBright · 2015-11-05T04:39:40Z

Can you post a screenshot yourself?

I removed it. But there was one dropdown, not 4, and it is as I said. No stable branch.

CyberShadow · 2015-11-05T12:06:39Z

Then you're on the wrong page. Read my comments above.

MartinNowak reviewed Nov 3, 2015
View reviewed changes

WalterBright force-pushed the fix15272 branch from fa21f48 to e98a9e7 Compare November 3, 2015 02:41

WalterBright force-pushed the fix15272 branch from e98a9e7 to aaea5aa Compare November 3, 2015 03:23

fix Issue 15272 - [2.069-rc2,inline] nothing written to output when -…

aaea5aa

…inline is set

MartinNowak closed this Nov 3, 2015

fix Issue 15272 - [2.069-rc2,inline] nothing written to output when -… #5253

fix Issue 15272 - [2.069-rc2,inline] nothing written to output when -… #5253

Conversation

WalterBright commented Nov 2, 2015

MartinNowak commented Nov 2, 2015

MartinNowak Nov 3, 2015

Choose a reason for hiding this comment

WalterBright Nov 3, 2015

Choose a reason for hiding this comment

WalterBright commented Nov 3, 2015

WalterBright commented Nov 3, 2015

WalterBright commented Nov 3, 2015

WalterBright commented Nov 3, 2015

aG0aep6G commented Nov 3, 2015

WalterBright commented Nov 3, 2015

CyberShadow commented Nov 3, 2015

WalterBright commented Nov 3, 2015

CyberShadow commented Nov 3, 2015

WalterBright commented Nov 3, 2015

WalterBright commented Nov 3, 2015

CyberShadow commented Nov 3, 2015

WalterBright commented Nov 3, 2015

WalterBright commented Nov 3, 2015

CyberShadow commented Nov 3, 2015

WalterBright commented Nov 3, 2015

CyberShadow commented Nov 3, 2015

WalterBright commented Nov 3, 2015

CyberShadow commented Nov 3, 2015

WalterBright commented Nov 3, 2015

CyberShadow commented Nov 3, 2015

MartinNowak commented Nov 3, 2015

WalterBright commented Nov 4, 2015

yebblies commented Nov 4, 2015

CyberShadow commented Nov 4, 2015

WalterBright commented Nov 4, 2015

WalterBright commented Nov 4, 2015

CyberShadow commented Nov 4, 2015

CyberShadow commented Nov 4, 2015

yebblies commented Nov 5, 2015

CyberShadow commented Nov 5, 2015

WalterBright commented Nov 5, 2015

CyberShadow commented Nov 5, 2015