Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
Hi Junio, On Tue, 21 Jun 2016, Junio C Hamano wrote: > Junio C Hamano writes: > > > Junio C Hamano writes: > > > >> Johannes Schindelin writes: > >> > >>> We are about to teach the log_tree machinery to reuse the diffopt.file > >>> setting to output to a file stream different from stdout. > >>> > >>> This means that builtin am can no longer ask the diff machinery to > >>> close the file when actually calling the log_tree machinery (which > >>> wants to flush the very same file stream that would then already be > >>> closed). > >> > >> Sorry for being slow, but I am not sure why the first paragraph has > >> to mean the second paragraph. This existing caller opens a new > >> stream, sets .fp to it, and expects that the log_tree_commit() to > >> close it if told by setting .close_file to true, all of which sounds > >> sensible. > >> > >> If a codepath wants to use the same stream for two or more calls to > >> log_tree by pointing the stream with .fp, it would be of course a > >> problem for the caller to set .close_file to true in its first call, > >> as .fp will be closed and no longer usable for second and subsequent > >> call, and that would be a bug, but for a single-shot call it feels > >> entirely a sensible request to make, no? > >> > >> Obviously you have looked at the codepaths involved a lot longer > >> than I did, and I do not doubt your conclusion, but I cannot quite > >> convince myself with the above explanation. > >> > >> The option parser of "git diff" family sets ->close_file to true > >> when the --output option is given. > >> > >> Wouldn't this patch break "git log --output=foo -3"? > > > > I wonder if the right approach is to stop using .close_file > > everywhere. > > > > With this "do not set .close_file if you use log_tree_commit()", > > "git log --output=/dev/stdout -3" gets broken, but removing that > > check is not sufficient to correct the same command with "-p", as > > letting .close_file to close the output file after finishing a > > single diff would mean that subsequent write to the same file > > descriptor will trigger a failure. > > We could say "git log --output=foo -3 [-p]" without any of your > patches is already broken, and it is a valid excuse to take this > change that we are not making things worse with it. > > It is just 3/9 is a logical first step to correct that exact > problem, i.e. some codepaths, even though there is a place that > holds the output stream and command line parser does prepare one for > "foo" when --output=foo is given, ignore it and send thigns to the > standard output stream. You might not have written 3/9 in order to > fix that "git log --output=foo" problem, but a fix for it should > look exactly like your 3/9, I would think. > > And it is sad that this step makes that fix impossible. Okay, I ended up seeing that there is no way I can avoid Doing The Right Thing. It is not quite as pretty as I hoped it would be (callers of log_tree_commit() that want to call it in a loop still have to override close_file manually), but it does produce a nicer story. Please see the fixes in the latest iteration I just sent out. Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
Junio C Hamano writes: > Junio C Hamano writes: > >> Johannes Schindelin writes: >> >>> We are about to teach the log_tree machinery to reuse the diffopt.file >>> setting to output to a file stream different from stdout. >>> >>> This means that builtin am can no longer ask the diff machinery to >>> close the file when actually calling the log_tree machinery (which >>> wants to flush the very same file stream that would then already be >>> closed). >> >> Sorry for being slow, but I am not sure why the first paragraph has >> to mean the second paragraph. This existing caller opens a new >> stream, sets .fp to it, and expects that the log_tree_commit() to >> close it if told by setting .close_file to true, all of which sounds >> sensible. >> >> If a codepath wants to use the same stream for two or more calls to >> log_tree by pointing the stream with .fp, it would be of course a >> problem for the caller to set .close_file to true in its first call, >> as .fp will be closed and no longer usable for second and subsequent >> call, and that would be a bug, but for a single-shot call it feels >> entirely a sensible request to make, no? >> >> Obviously you have looked at the codepaths involved a lot longer >> than I did, and I do not doubt your conclusion, but I cannot quite >> convince myself with the above explanation. >> >> The option parser of "git diff" family sets ->close_file to true >> when the --output option is given. >> >> Wouldn't this patch break "git log --output=foo -3"? > > I wonder if the right approach is to stop using .close_file > everywhere. > > With this "do not set .close_file if you use log_tree_commit()", > "git log --output=/dev/stdout -3" gets broken, but removing that > check is not sufficient to correct the same command with "-p", as > letting .close_file to close the output file after finishing a > single diff would mean that subsequent write to the same file > descriptor will trigger a failure. We could say "git log --output=foo -3 [-p]" without any of your patches is already broken, and it is a valid excuse to take this change that we are not making things worse with it. It is just 3/9 is a logical first step to correct that exact problem, i.e. some codepaths, even though there is a place that holds the output stream and command line parser does prepare one for "foo" when --output=foo is given, ignore it and send thigns to the standard output stream. You might not have written 3/9 in order to fix that "git log --output=foo" problem, but a fix for it should look exactly like your 3/9, I would think. And it is sad that this step makes that fix impossible. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
Junio C Hamano writes: > Johannes Schindelin writes: > >> We are about to teach the log_tree machinery to reuse the diffopt.file >> setting to output to a file stream different from stdout. >> >> This means that builtin am can no longer ask the diff machinery to >> close the file when actually calling the log_tree machinery (which >> wants to flush the very same file stream that would then already be >> closed). > > Sorry for being slow, but I am not sure why the first paragraph has > to mean the second paragraph. This existing caller opens a new > stream, sets .fp to it, and expects that the log_tree_commit() to > close it if told by setting .close_file to true, all of which sounds > sensible. > > If a codepath wants to use the same stream for two or more calls to > log_tree by pointing the stream with .fp, it would be of course a > problem for the caller to set .close_file to true in its first call, > as .fp will be closed and no longer usable for second and subsequent > call, and that would be a bug, but for a single-shot call it feels > entirely a sensible request to make, no? > > Obviously you have looked at the codepaths involved a lot longer > than I did, and I do not doubt your conclusion, but I cannot quite > convince myself with the above explanation. > > The option parser of "git diff" family sets ->close_file to true > when the --output option is given. > > Wouldn't this patch break "git log --output=foo -3"? I wonder if the right approach is to stop using .close_file everywhere. With this "do not set .close_file if you use log_tree_commit()", "git log --output=/dev/stdout -3" gets broken, but removing that check is not sufficient to correct the same command with "-p", as letting .close_file to close the output file after finishing a single diff would mean that subsequent write to the same file descriptor will trigger a failure. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
Johannes Schindelin writes: > We are about to teach the log_tree machinery to reuse the diffopt.file > setting to output to a file stream different from stdout. > > This means that builtin am can no longer ask the diff machinery to > close the file when actually calling the log_tree machinery (which > wants to flush the very same file stream that would then already be > closed). Sorry for being slow, but I am not sure why the first paragraph has to mean the second paragraph. This existing caller opens a new stream, sets .fp to it, and expects that the log_tree_commit() to close it if told by setting .close_file to true, all of which sounds sensible. If a codepath wants to use the same stream for two or more calls to log_tree by pointing the stream with .fp, it would be of course a problem for the caller to set .close_file to true in its first call, as .fp will be closed and no longer usable for second and subsequent call, and that would be a bug, but for a single-shot call it feels entirely a sensible request to make, no? Obviously you have looked at the codepaths involved a lot longer than I did, and I do not doubt your conclusion, but I cannot quite convince myself with the above explanation. The option parser of "git diff" family sets ->close_file to true when the --output option is given. Wouldn't this patch break "git log --output=foo -3"? > To stave off similar problems in the future, report it as a bug if > log_tree_commit() is called with a non-zero diffopt.close_file. > > Signed-off-by: Johannes Schindelin > --- > builtin/am.c | 6 -- > log-tree.c | 3 +++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/builtin/am.c b/builtin/am.c > index 0e28a62..47d78aa 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -1437,6 +1437,7 @@ static int write_commit_patch(const struct am_state > *state, struct commit *commi > { > struct rev_info rev_info; > FILE *fp; > + int res; > > fp = fopen(am_path(state, "patch"), "w"); > if (!fp) > @@ -1453,10 +1454,11 @@ static int write_commit_patch(const struct am_state > *state, struct commit *commi > DIFF_OPT_SET(&rev_info.diffopt, FULL_INDEX); > rev_info.diffopt.use_color = 0; > rev_info.diffopt.file = fp; > - rev_info.diffopt.close_file = 1; > add_pending_object(&rev_info, &commit->object, ""); > diff_setup_done(&rev_info.diffopt); > - return log_tree_commit(&rev_info, commit); > + res = log_tree_commit(&rev_info, commit); > + fclose(fp); > + return res; > } > > /** > diff --git a/log-tree.c b/log-tree.c > index 78a5381..dc0180d 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -864,6 +864,9 @@ int log_tree_commit(struct rev_info *opt, struct commit > *commit) > struct log_info log; > int shown; > > + if (opt->diffopt.close_file) > + die("BUG: close_file is incompatible with log_tree_commit()"); > + > log.commit = commit; > log.parent = NULL; > opt->loginfo = &log; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html