1299530 - TraceLogger: Remove outdated redirection file when renaming

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

I think I ran into a problem where I renamed something, and it kept looking for the old file.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: TraceLogger: Remove outdated redirection file when renaming

Comment 2

[Hannes Verschore [:h4writer]]

Comment on attachment 8786790 [details] [diff] [review]
TraceLogger: Remove outdated redirection file when renaming

Review of attachment 8786790 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools_v2/rename.py
@@ +99,5 @@
>      with open(new_redir_file, "w") as fp:
>          fp.write("\"%s.json\"" % basename(new_name))
> +    if not os.path.samefile(redir_file, new_redir_file):
> +        if not args.keep:
> +            os.remove(redir_file)

Isn't that what is happening on line 95? 5 lines before this code?
I moved it to that place to make sure we didn't remove it after creating it here.
As a result I think this code is obsolete?

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> Comment on attachment 8786790 [details] [diff] [review]
> TraceLogger: Remove outdated redirection file when renaming
> 
> Review of attachment 8786790 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools_v2/rename.py
> @@ +99,5 @@
> >      with open(new_redir_file, "w") as fp:
> >          fp.write("\"%s.json\"" % basename(new_name))
> > +    if not os.path.samefile(redir_file, new_redir_file):
> > +        if not args.keep:
> > +            os.remove(redir_file)
> 
> Isn't that what is happening on line 95? 5 lines before this code?
> I moved it to that place to make sure we didn't remove it after creating it
> here.
> As a result I think this code is obsolete?

Oh! Sorry, I assumed that it got dropped in the merge, and didn't notice you'd moved it up.

My general reasoning is that when initially using this stuff, I was getting errors in the middle of the renaming, but it had already deleted some stuff and so I was left with mangled directory contents. So this was an application of the general "delete as late as possible, in case something went wrong." (And yeah, I noticed that this might delete the file you just created, and specifically checked for that.)

But the only way I can see that this would fail is if you run out of disk space or there were permissions problems or something, and you'd almost certainly error out earlier. So I think you're right, we can just drop this patch. Your version is simpler since it can skip the os.path.samefile check.