Hey Eric, On Sat, Jun 11, 2016 at 12:44 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Fri, Jun 10, 2016 at 9:39 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: >> On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: >>> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: >>>> Reimplement `is_expected_rev` shell function in C. This will further be >>>> called from `check_expected_revs` function. This is a quite small >>>> function thus subcommand facility is redundant. >>> >>> This patch should be squashed into patch 2/2, as it is otherwise >>> pointless without that patch, and merely adds dead code. >> >> Sure I will squash and will explain it in the commit message. > > Explain what in the commit message? If anything, I'd expect the commit > message to shrink since you won't need to explain anymore that this > function is split out. Yes I would remove the part where it is explained that this function is split out. I will just explain that 2 functions are converted in 1 commit. >>>> + if (!file_exists(git_path_bisect_expected_rev())) >>>> + return 0; >>> >>> Invoking file_exists() seems unnecessarily redundant when you can >>> discern effectively the same by checking the return value of >>> strbuf_read_file() below. I'd drop the file_exists() check altogether. >> >> I wanted to imitate the code. But I guess it would actually be better >> if I drop this file_exists(). > > There is a bit of a lesson to be learned by this example. While it's > true that the C conversion should retain the behavior of the original > shell code, that does not mean blindly mirroring the implementation > line for line is a good idea. A couple things to take into > consideration: > > There are idiomatic ways of doing things in each language. What is > idiomatic in shell is not necessarily so in C. The C conversion should > employ C idioms and flow in a way which is natural for C code. > > Consider what the original shell code is doing at a higher level than > merely by reading it line-by-line. In the case in question, the code > is: > > test -f "$GIT_DIR/BISECT_EXPECTED_REV" && > test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV") > > While it's true that it's asking "does the file exist and is its value > the same as $1", the 'test -f' avoids a "file not found" error from > the $(cat ...) invocation. Since the return value of > strbuf_read_file() effectively encapsulates the "does the file exist" > check, a separate check isn't really needed. True. I will keep this in mind. >>>> + if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0)) >>>> + return 0; >>> >>> What exactly is this trying to do? Considering that strbuf_read_file() >>> returns -1 upon error, otherwise the number of bytes read, if I'm >>> reading this correctly, is_expected_rev() returns false if >>> strbuf_read_file() encounters an error (which is fine) but also when >>> it successfully reads the file and its content length is non-zero >>> (which is very odd). >>> >>>> + strbuf_trim(&actual_hex); >>>> + return !strcmp(actual_hex.buf, expected_hex); >>> >>> Thus, it only ever gets to this point if the file exists but is empty, >>> which is very unlikely to match 'expected_hex'. I could understand it >>> if you checked the result of strbuf_read_file() with <0 or even <=0, >>> but the current code doesn't make sense to me. >>> >>> Am I misunderstanding? >> >> Definitely not. Thanks for pointing it out. :) It went off my head >> that strbuf_read_file returns the bytes it reads. Also the code >> comment regarding strbuf_read_file does not mention it which probably >> misguided me. I should also send a fixing patch so that someone else >> does not fall into this like I did. > > Out of curiosity, did the test suite pass with this patch applied? > This is such an egregious bug that it's hard to imagine the tests > passing, but if they did, then that may be a good indication that > coverage is too sparse and ought to be improved. Yes the test suite passed perfectly. I have inculcated the habit of running the whole test suite before sending patches. Yes some parts of a test suite seem to be missing. How about I do it in the end? By this I won't have to setup yet another coverage tool for shell script. I can use the coverage tool by GNU to test the coverage after bisect is a C code. Till that time the patches can reside in the pu branch. Regards, Pranit Bauva -- 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