|
|
|
Created:
11 years, 9 months ago by mra Modified:
11 years, 8 months ago CC:
golang-codereviews Visibility:
Public. |
cmd/ld: don't delete output binary if not "ordinary" file.
e.g., don't delete /dev/null. this fix inspired by gnu libiberty,
unlink-if-ordinary.c.
Fixes issue 7563
Patch Set 1 #Patch Set 2 : diff -r 32b8c26e4414 https://code.google.com/p/go #Patch Set 3 : diff -r 32b8c26e4414 https://code.google.com/p/go #Patch Set 4 : diff -r 2bce43d48503 https://code.google.com/p/go #Patch Set 5 : diff -r 2bce43d48503 https://code.google.com/p/go #
Total comments: 2
Patch Set 6 : code review 76580044: cmd/ld: don't delete output binary if not "ordinary" file. #Total messages: 11
|
mra
Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
|
11 years, 9 months ago (2014年03月17日 16:16:03 UTC) #1 |
Hello golang-codereviews@googlegroups.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
thanks in advance, and my motivation for submitting this is that when editing golang code in emacs as root user (as i usually do), misc/emacs/go-mode.el causes deletion of /dev/null. this was completely unexpected and unusual to encounter. best regards, mike. On Mon, Mar 17, 2014 at 12:16 PM, <mra@xoba.com> wrote: > Reviewers: golang-codereviews, > > Message: > Hello golang-codereviews@googlegroups.com (cc: > golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > src/cmd/ld/lib.c: don't delete /dev/null if that's the binary's output > path > > Fixes issue 7563 > > Please review this at https://codereview.appspot.com/76580044/ > > Affected files (+3, -1 lines): > M src/cmd/ld/lib.c > > > Index: src/cmd/ld/lib.c > =================================================================== > --- a/src/cmd/ld/lib.c > +++ b/src/cmd/ld/lib.c > @@ -107,7 +107,9 @@ > // recently run) binary, so remove the output file before writing > it. > // On Windows 7, remove() can force the following create() to fail. > #ifndef _WIN32 > - remove(outfile); > + if (strcmp(outfile, "/dev/null") != 0) { // please, never remove > /dev/null :-) > + remove(outfile); > + } > #endif > cout = create(outfile, 1, 0775); > if(cout < 0) { > > >
Thanks, but this is not the right fix. I outlined the right fix in the bug report.
Hello golang-codereviews@googlegroups.com, iant@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
i forgot to say PTAL, thanks, & i'll be happy to revise as folks think best. On 2014年03月17日 19:50:08, mra wrote: > Hello mailto:golang-codereviews@googlegroups.com, mailto:iant@golang.org (cc: > mailto:golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go
The CL description should start with just cmd/ld (you can change it with "hg change", or using the web interface). https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c File src/cmd/ld/lib.c (right): https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode111 src/cmd/ld/lib.c:111: struct stat st; Please start a new block so that the local variable is in a block by itself. In this code we don't assume C99. https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode112 src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG (st.st_mode) || S_ISLNK (st.st_mode))) No space before left parenthesis, four times.
my apologies, i'm abandoning this CL and have replaced it with https://codereview.appspot.com/76810045 because it seems this one can not be edited anymore? On 2014年03月20日 22:41:51, iant wrote: > The CL description should start with just cmd/ld (you can change it with "hg > change", or using the web interface). > > https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c > File src/cmd/ld/lib.c (right): > > https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode111 > src/cmd/ld/lib.c:111: struct stat st; > Please start a new block so that the local variable is in a block by itself. In > this code we don't assume C99. > > https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode112 > src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG (st.st_mode) || > S_ISLNK (st.st_mode))) > No space before left parenthesis, four times.
hg change -d 6810045 <https://codereview.appspot.com/76810045> On Sat, Mar 22, 2014 at 11:14 AM, <mra@xoba.com> wrote: > my apologies, i'm abandoning this CL and have replaced it with > https://codereview.appspot.com/76810045 because it seems this one can > not be edited anymore? > > > On 2014年03月20日 22:41:51, iant wrote: > >> The CL description should start with just cmd/ld (you can change it >> > with "hg > >> change", or using the web interface). >> > > https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c >> File src/cmd/ld/lib.c (right): >> > > > https://codereview.appspot.com/76580044/diff/70001/src/ > cmd/ld/lib.c#newcode111 > >> src/cmd/ld/lib.c:111: struct stat st; >> Please start a new block so that the local variable is in a block by >> > itself. In > >> this code we don't assume C99. >> > > > https://codereview.appspot.com/76580044/diff/70001/src/ > cmd/ld/lib.c#newcode112 > >> src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG >> > (st.st_mode) || > >> S_ISLNK (st.st_mode))) >> No space before left parenthesis, four times. >> > > > https://codereview.appspot.com/76580044/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. >
I'm sorry, the correct command is hg change -d 76580044 On Sat, Mar 22, 2014 at 9:18 PM, Dave Cheney <dave@cheney.net> wrote: > hg change -d 6810045 > > > On Sat, Mar 22, 2014 at 11:14 AM, <mra@xoba.com> wrote: >> >> my apologies, i'm abandoning this CL and have replaced it with >> https://codereview.appspot.com/76810045 because it seems this one can >> not be edited anymore? >> >> >> On 2014年03月20日 22:41:51, iant wrote: >>> >>> The CL description should start with just cmd/ld (you can change it >> >> with "hg >>> >>> change", or using the web interface). >> >> >>> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c >>> File src/cmd/ld/lib.c (right): >> >> >> >> >> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode111 >>> >>> src/cmd/ld/lib.c:111: struct stat st; >>> Please start a new block so that the local variable is in a block by >> >> itself. In >>> >>> this code we don't assume C99. >> >> >> >> >> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode112 >>> >>> src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG >> >> (st.st_mode) || >>> >>> S_ISLNK (st.st_mode))) >>> No space before left parenthesis, four times. >> >> >> >> https://codereview.appspot.com/76580044/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "golang-codereviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to golang-codereviews+unsubscribe@googlegroups.com. >> For more options, visit https://groups.google.com/d/optout. > >
thanks, i did try to delete it yesterday, but got the error "abort: cannot change non-local CL 76580044", even after running "hg clpatch 76580044" on a fresh repository, which also gave an error: "abort: codereview issue 76580044 has no diff". so, i then started to suspect that somehow the CL got into an unusual state preventing further edits or deletion? perhaps some sort of mistake i made earlier removed the diff per se, causing this? On Sat, Mar 22, 2014 at 6:19 AM, Dave Cheney <dave@cheney.net> wrote: > I'm sorry, the correct command is > > hg change -d 76580044 > > On Sat, Mar 22, 2014 at 9:18 PM, Dave Cheney <dave@cheney.net> wrote: > > hg change -d 6810045 > > > > > > On Sat, Mar 22, 2014 at 11:14 AM, <mra@xoba.com> wrote: > >> > >> my apologies, i'm abandoning this CL and have replaced it with > >> https://codereview.appspot.com/76810045 because it seems this one can > >> not be edited anymore? > >> > >> > >> On 2014年03月20日 22:41:51, iant wrote: > >>> > >>> The CL description should start with just cmd/ld (you can change it > >> > >> with "hg > >>> > >>> change", or using the web interface). > >> > >> > >>> https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c > >>> File src/cmd/ld/lib.c (right): > >> > >> > >> > >> > >> > https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode111 > >>> > >>> src/cmd/ld/lib.c:111: struct stat st; > >>> Please start a new block so that the local variable is in a block by > >> > >> itself. In > >>> > >>> this code we don't assume C99. > >> > >> > >> > >> > >> > https://codereview.appspot.com/76580044/diff/70001/src/cmd/ld/lib.c#newcode112 > >>> > >>> src/cmd/ld/lib.c:112: if (lstat (outfile, &st) == 0 && (S_ISREG > >> > >> (st.st_mode) || > >>> > >>> S_ISLNK (st.st_mode))) > >>> No space before left parenthesis, four times. > >> > >> > >> > >> https://codereview.appspot.com/76580044/ > >> > >> -- > >> You received this message because you are subscribed to the Google > Groups > >> "golang-codereviews" group. > >> To unsubscribe from this group and stop receiving emails from it, send > an > >> email to golang-codereviews+unsubscribe@googlegroups.com. > >> For more options, visit https://groups.google.com/d/optout. > > > > >