* [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff()
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
@ 2016-06-21 10:34 ` Johannes Schindelin
2016-06-21 18:59 ` Junio C Hamano
2016-06-21 10:34 ` [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery Johannes Schindelin
` (9 subsequent siblings)
10 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Paul Tan
It is never a good idea to ignore errors, so let's handle them.
While at it, handle fopen() errors more gently (i.e. give the caller a
chance to do something about it rather than die()ing).
Note: there are more places in the builtin am code that ignore
errors returned from library functions. Fixing those is outside the
purview of the current patch series, though.
Cc: Paul Tan <pyokagan@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/am.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b..0e28a62 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1433,12 +1433,15 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
/**
* Writes `commit` as a patch to the state directory's "patch" file.
*/
-static void write_commit_patch(const struct am_state *state, struct commit *commit)
+static int write_commit_patch(const struct am_state *state, struct commit *commit)
{
struct rev_info rev_info;
FILE *fp;
- fp = xfopen(am_path(state, "patch"), "w");
+ fp = fopen(am_path(state, "patch"), "w");
+ if (!fp)
+ return error(_("Could not open %s for writing"),
+ am_path(state, "patch"));
init_revisions(&rev_info, NULL);
rev_info.diff = 1;
rev_info.abbrev = 0;
@@ -1453,7 +1456,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
rev_info.diffopt.close_file = 1;
add_pending_object(&rev_info, &commit->object, "");
diff_setup_done(&rev_info.diffopt);
- log_tree_commit(&rev_info, commit);
+ return log_tree_commit(&rev_info, commit);
}
/**
@@ -1501,13 +1504,14 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
unsigned char commit_sha1[GIT_SHA1_RAWSZ];
if (get_mail_commit_sha1(commit_sha1, mail) < 0)
- die(_("could not parse %s"), mail);
+ return error(_("could not parse %s"), mail);
commit = lookup_commit_or_die(commit_sha1, mail);
get_commit_info(state, commit);
- write_commit_patch(state, commit);
+ if (write_commit_patch(state, commit) < 0)
+ return -1;
hashcpy(state->orig_commit, commit_sha1);
write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff()
2016-06-21 10:34 ` [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff() Johannes Schindelin
@ 2016-06-21 18:59 ` Junio C Hamano
2016-06-22 12:21 ` Johannes Schindelin
0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2016-06-21 18:59 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine, Paul Tan
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Note: there are more places in the builtin am code that ignore
> errors returned from library functions. Fixing those is outside the
> purview of the current patch series, though.
The caller of parse_mail() and parse_mail_rebase() is not prepared
to see an error code in the returned value from these function,
which are to return a boolean telling the caller to skip or use the
patch file. At least the caller needs to notice negative return and
made to die/exit(128), instead of silently skipping a corrupt or
unopenable patch, no? Otherwise this will change the behaviour.
> Cc: Paul Tan <pyokagan@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/am.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 3dfe70b..0e28a62 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1433,12 +1433,15 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
> /**
> * Writes `commit` as a patch to the state directory's "patch" file.
> */
> -static void write_commit_patch(const struct am_state *state, struct commit *commit)
> +static int write_commit_patch(const struct am_state *state, struct commit *commit)
> {
> struct rev_info rev_info;
> FILE *fp;
>
> - fp = xfopen(am_path(state, "patch"), "w");
> + fp = fopen(am_path(state, "patch"), "w");
> + if (!fp)
> + return error(_("Could not open %s for writing"),
> + am_path(state, "patch"));
> init_revisions(&rev_info, NULL);
> rev_info.diff = 1;
> rev_info.abbrev = 0;
> @@ -1453,7 +1456,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm
> rev_info.diffopt.close_file = 1;
> add_pending_object(&rev_info, &commit->object, "");
> diff_setup_done(&rev_info.diffopt);
> - log_tree_commit(&rev_info, commit);
> + return log_tree_commit(&rev_info, commit);
> }
>
> /**
> @@ -1501,13 +1504,14 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
> unsigned char commit_sha1[GIT_SHA1_RAWSZ];
>
> if (get_mail_commit_sha1(commit_sha1, mail) < 0)
> - die(_("could not parse %s"), mail);
> + return error(_("could not parse %s"), mail);
>
> commit = lookup_commit_or_die(commit_sha1, mail);
>
> get_commit_info(state, commit);
>
> - write_commit_patch(state, commit);
> + if (write_commit_patch(state, commit) < 0)
> + return -1;
>
> hashcpy(state->orig_commit, commit_sha1);
> write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff()
2016-06-21 18:59 ` Junio C Hamano
@ 2016-06-22 12:21 ` Johannes Schindelin
0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 12:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Paul Tan
Hi Junio,
On Tue, 21 Jun 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > Note: there are more places in the builtin am code that ignore
> > errors returned from library functions. Fixing those is outside the
> > purview of the current patch series, though.
>
> The caller of parse_mail() and parse_mail_rebase() is not prepared
> to see an error code in the returned value from these function,
> which are to return a boolean telling the caller to skip or use the
> patch file. At least the caller needs to notice negative return and
> made to die/exit(128), instead of silently skipping a corrupt or
> unopenable patch, no? Otherwise this will change the behaviour.
Yeah, that is another rabbit hole I really do not want to dive in.
Will leave it alone,
Dscho
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
2016-06-21 10:34 ` [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff() Johannes Schindelin
@ 2016-06-21 10:34 ` Johannes Schindelin
2016-06-21 18:14 ` Junio C Hamano
2016-06-21 10:34 ` [PATCH v3 3/9] log-tree: respect diffopt's configured output file stream Johannes Schindelin
` (8 subsequent siblings)
10 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Paul Tan
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).
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 <johannes.schindelin@gmx.de>
---
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;
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
2016-06-21 10:34 ` [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery Johannes Schindelin
@ 2016-06-21 18:14 ` Junio C Hamano
2016-06-21 19:05 ` Junio C Hamano
0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2016-06-21 18:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine, Paul Tan
Johannes Schindelin <johannes.schindelin@gmx.de> 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 <johannes.schindelin@gmx.de>
> ---
> 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;
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
2016-06-21 18:14 ` Junio C Hamano
@ 2016-06-21 19:05 ` Junio C Hamano
2016-06-21 19:32 ` Junio C Hamano
0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2016-06-21 19:05 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine, Paul Tan
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> 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.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
2016-06-21 19:05 ` Junio C Hamano
@ 2016-06-21 19:32 ` Junio C Hamano
2016-06-22 15:17 ` Johannes Schindelin
0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2016-06-21 19:32 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine, Paul Tan
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> 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.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery
2016-06-21 19:32 ` Junio C Hamano
@ 2016-06-22 15:17 ` Johannes Schindelin
0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Paul Tan
Hi Junio,
On Tue, 21 Jun 2016, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> Johannes Schindelin <johannes.schindelin@gmx.de> 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
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v3 3/9] log-tree: respect diffopt's configured output file stream
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
2016-06-21 10:34 ` [PATCH v3 1/9] am: stop ignoring errors reported by log_tree_diff() Johannes Schindelin
2016-06-21 10:34 ` [PATCH v3 2/9] Disallow diffopt.close_file when using the log_tree machinery Johannes Schindelin
@ 2016-06-21 10:34 ` Johannes Schindelin
2016-06-21 10:35 ` [PATCH v3 4/9] line-log: " Johannes Schindelin
` (7 subsequent siblings)
10 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
The diff options already know how to print the output anywhere else
than stdout. The same is needed for log output in general, e.g.
when writing patches to files in `git format-patch`. Let's allow
users to use log_tree_commit() *without* changing global state via
freopen().
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
log-tree.c | 64 +++++++++++++++++++++++++++++++-------------------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index dc0180d..530297d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -159,12 +159,12 @@ void load_ref_decorations(int flags)
}
}
-static void show_parents(struct commit *commit, int abbrev)
+static void show_parents(struct commit *commit, int abbrev, FILE *file)
{
struct commit_list *p;
for (p = commit->parents; p ; p = p->next) {
struct commit *parent = p->item;
- printf(" %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
+ fprintf(file, " %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
}
}
@@ -172,7 +172,7 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
{
struct commit_list *p = lookup_decoration(&opt->children, &commit->object);
for ( ; p; p = p->next) {
- printf(" %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
+ fprintf(opt->diffopt.file, " %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
}
}
@@ -286,11 +286,11 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
struct strbuf sb = STRBUF_INIT;
if (opt->show_source && commit->util)
- printf("\t%s", (char *) commit->util);
+ fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
if (!opt->show_decorations)
return;
format_decorations(&sb, commit, opt->diffopt.use_color);
- fputs(sb.buf, stdout);
+ fputs(sb.buf, opt->diffopt.file);
strbuf_release(&sb);
}
@@ -364,18 +364,18 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
subject = "Subject: ";
}
- printf("From %s Mon Sep 17 00:00:00 2001\n", name);
+ fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
graph_show_oneline(opt->graph);
if (opt->message_id) {
- printf("Message-Id: <%s>\n", opt->message_id);
+ fprintf(opt->diffopt.file, "Message-Id: <%s>\n", opt->message_id);
graph_show_oneline(opt->graph);
}
if (opt->ref_message_ids && opt->ref_message_ids->nr > 0) {
int i, n;
n = opt->ref_message_ids->nr;
- printf("In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
+ fprintf(opt->diffopt.file, "In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
for (i = 0; i < n; i++)
- printf("%s<%s>\n", (i > 0 ? "\t" : "References: "),
+ fprintf(opt->diffopt.file, "%s<%s>\n", (i > 0 ? "\t" : "References: "),
opt->ref_message_ids->items[i].string);
graph_show_oneline(opt->graph);
}
@@ -432,7 +432,7 @@ static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
reset = diff_get_color_opt(&opt->diffopt, DIFF_RESET);
while (*bol) {
eol = strchrnul(bol, '\n');
- printf("%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
+ fprintf(opt->diffopt.file, "%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
*eol ? "\n" : "");
graph_show_oneline(opt->graph);
bol = (*eol) ? (eol + 1) : eol;
@@ -553,17 +553,17 @@ void show_log(struct rev_info *opt)
if (!opt->graph)
put_revision_mark(opt, commit);
- fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), stdout);
+ fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), opt->diffopt.file);
if (opt->print_parents)
- show_parents(commit, abbrev_commit);
+ show_parents(commit, abbrev_commit, opt->diffopt.file);
if (opt->children.name)
show_children(opt, commit, abbrev_commit);
show_decorations(opt, commit);
if (opt->graph && !graph_is_commit_finished(opt->graph)) {
- putchar('\n');
+ putc('\n', opt->diffopt.file);
graph_show_remainder(opt->graph);
}
- putchar(opt->diffopt.line_termination);
+ putc(opt->diffopt.line_termination, opt->diffopt.file);
return;
}
@@ -589,7 +589,7 @@ void show_log(struct rev_info *opt)
if (opt->diffopt.line_termination == '\n' &&
!opt->missing_newline)
graph_show_padding(opt->graph);
- putchar(opt->diffopt.line_termination);
+ putc(opt->diffopt.line_termination, opt->diffopt.file);
}
opt->shown_one = 1;
@@ -607,28 +607,28 @@ void show_log(struct rev_info *opt)
log_write_email_headers(opt, commit, &ctx.subject, &extra_headers,
&ctx.need_8bit_cte);
} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
- fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), stdout);
+ fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);
if (opt->commit_format != CMIT_FMT_ONELINE)
- fputs("commit ", stdout);
+ fputs("commit ", opt->diffopt.file);
if (!opt->graph)
put_revision_mark(opt, commit);
fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit),
- stdout);
+ opt->diffopt.file);
if (opt->print_parents)
- show_parents(commit, abbrev_commit);
+ show_parents(commit, abbrev_commit, opt->diffopt.file);
if (opt->children.name)
show_children(opt, commit, abbrev_commit);
if (parent)
- printf(" (from %s)",
+ fprintf(opt->diffopt.file, " (from %s)",
find_unique_abbrev(parent->object.oid.hash,
abbrev_commit));
- fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), stdout);
+ fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
show_decorations(opt, commit);
if (opt->commit_format == CMIT_FMT_ONELINE) {
- putchar(' ');
+ putc(' ', opt->diffopt.file);
} else {
- putchar('\n');
+ putc('\n', opt->diffopt.file);
graph_show_oneline(opt->graph);
}
if (opt->reflog_info) {
@@ -702,7 +702,7 @@ void show_log(struct rev_info *opt)
}
if (opt->show_log_size) {
- printf("log size %i\n", (int)msgbuf.len);
+ fprintf(opt->diffopt.file, "log size %i\n", (int)msgbuf.len);
graph_show_oneline(opt->graph);
}
@@ -718,11 +718,11 @@ void show_log(struct rev_info *opt)
if (opt->graph)
graph_show_commit_msg(opt->graph, &msgbuf);
else
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len, opt->diffopt.file);
if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
if (!opt->missing_newline)
graph_show_padding(opt->graph);
- putchar(opt->diffopt.line_termination);
+ putc(opt->diffopt.line_termination, opt->diffopt.file);
}
strbuf_release(&msgbuf);
@@ -759,7 +759,7 @@ int log_tree_diff_flush(struct rev_info *opt)
struct strbuf *msg = NULL;
msg = opt->diffopt.output_prefix(&opt->diffopt,
opt->diffopt.output_prefix_data);
- fwrite(msg->buf, msg->len, 1, stdout);
+ fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
}
/*
@@ -774,8 +774,8 @@ int log_tree_diff_flush(struct rev_info *opt)
*/
if (!opt->shown_dashes &&
(pch & opt->diffopt.output_format) == pch)
- printf("---");
- putchar('\n');
+ fprintf(opt->diffopt.file, "---");
+ putc('\n', opt->diffopt.file);
}
}
diff_flush(&opt->diffopt);
@@ -875,7 +875,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
return line_log_print(opt, commit);
if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
- printf("\n%s\n", opt->break_bar);
+ fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
shown = log_tree_diff(opt, commit, &log);
if (!shown && opt->loginfo && opt->always_show_header) {
log.parent = NULL;
@@ -883,8 +883,8 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
shown = 1;
}
if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
- printf("\n%s\n", opt->break_bar);
+ fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
opt->loginfo = NULL;
- maybe_flush_or_die(stdout, "stdout");
+ maybe_flush_or_die(opt->diffopt.file, "stdout");
return shown;
}
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 4/9] line-log: respect diffopt's configured output file stream
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
` (2 preceding siblings ...)
2016-06-21 10:34 ` [PATCH v3 3/9] log-tree: respect diffopt's configured output file stream Johannes Schindelin
@ 2016-06-21 10:35 ` Johannes Schindelin
2016-06-21 10:35 ` [PATCH v3 5/9] graph: respect the diffopt.file setting Johannes Schindelin
` (6 subsequent siblings)
10 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
The diff machinery can optionally output to a file stream other than
stdout, by overriding diffopt.file. In such a case, the rest of the
log tree machinery should also write to that stream.
Currently, there is no user of the line level log that wants to
redirect output to a file. Therefore, one might argue that it is
superfluous to support that now. However, it is better to be
consistent now, rather than to face hard-to-debug problems later.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
line-log.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/line-log.c b/line-log.c
index bbe31ed..e62a7f4 100644
--- a/line-log.c
+++ b/line-log.c
@@ -841,7 +841,7 @@ static char *get_nth_line(long line, unsigned long *ends, void *data)
static void print_line(const char *prefix, char first,
long line, unsigned long *ends, void *data,
- const char *color, const char *reset)
+ const char *color, const char *reset, FILE *file)
{
char *begin = get_nth_line(line, ends, data);
char *end = get_nth_line(line+1, ends, data);
@@ -852,14 +852,14 @@ static void print_line(const char *prefix, char first,
had_nl = 1;
}
- fputs(prefix, stdout);
- fputs(color, stdout);
- putchar(first);
- fwrite(begin, 1, end-begin, stdout);
- fputs(reset, stdout);
- putchar('\n');
+ fputs(prefix, file);
+ fputs(color, file);
+ putc(first, file);
+ fwrite(begin, 1, end-begin, file);
+ fputs(reset, file);
+ putc('\n', file);
if (!had_nl)
- fputs("\\ No newline at end of file\n", stdout);
+ fputs("\\ No newline at end of file\n", file);
}
static char *output_prefix(struct diff_options *opt)
@@ -898,12 +898,12 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
fill_line_ends(pair->one, &p_lines, &p_ends);
fill_line_ends(pair->two, &t_lines, &t_ends);
- printf("%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
- printf("%s%s--- %s%s%s\n", prefix, c_meta,
+ fprintf(opt->file, "%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
+ fprintf(opt->file, "%s%s--- %s%s%s\n", prefix, c_meta,
pair->one->sha1_valid ? "a/" : "",
pair->one->sha1_valid ? pair->one->path : "/dev/null",
c_reset);
- printf("%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
+ fprintf(opt->file, "%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
for (i = 0; i < range->ranges.nr; i++) {
long p_start, p_end;
long t_start = range->ranges.ranges[i].start;
@@ -945,7 +945,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
}
/* Now output a diff hunk for this range */
- printf("%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
+ fprintf(opt->file, "%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
prefix, c_frag,
p_start+1, p_end-p_start, t_start+1, t_end-t_start,
c_reset);
@@ -953,18 +953,18 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
int k;
for (; t_cur < diff->target.ranges[j].start; t_cur++)
print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
- c_context, c_reset);
+ c_context, c_reset, opt->file);
for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
print_line(prefix, '-', k, p_ends, pair->one->data,
- c_old, c_reset);
+ c_old, c_reset, opt->file);
for (; t_cur < diff->target.ranges[j].end && t_cur < t_end; t_cur++)
print_line(prefix, '+', t_cur, t_ends, pair->two->data,
- c_new, c_reset);
+ c_new, c_reset, opt->file);
j++;
}
for (; t_cur < t_end; t_cur++)
print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
- c_context, c_reset);
+ c_context, c_reset, opt->file);
}
free(p_ends);
@@ -977,7 +977,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
*/
static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
{
- puts(output_prefix(&rev->diffopt));
+ fprintf(rev->diffopt.file, "%s\n", output_prefix(&rev->diffopt));
while (range) {
dump_diff_hacky_one(rev, range);
range = range->next;
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 5/9] graph: respect the diffopt.file setting
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
` (3 preceding siblings ...)
2016-06-21 10:35 ` [PATCH v3 4/9] line-log: " Johannes Schindelin
@ 2016-06-21 10:35 ` Johannes Schindelin
2016-06-21 10:35 ` [PATCH v3 6/9] shortlog: support outputting to streams other than stdout Johannes Schindelin
` (5 subsequent siblings)
10 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
When the caller overrides diffopt.file (which defaults to stdout),
the diff machinery already redirects its output, and the graph display
should also write to that file.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
graph.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/graph.c b/graph.c
index 1350bdd..8ad8ba3 100644
--- a/graph.c
+++ b/graph.c
@@ -17,8 +17,8 @@
static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
/*
- * Print a strbuf to stdout. If the graph is non-NULL, all lines but the
- * first will be prefixed with the graph output.
+ * Print a strbuf. If the graph is non-NULL, all lines but the first will be
+ * prefixed with the graph output.
*
* If the strbuf ends with a newline, the output will end after this
* newline. A new graph line will not be printed after the final newline.
@@ -1193,9 +1193,10 @@ void graph_show_commit(struct git_graph *graph)
while (!shown_commit_line && !graph_is_commit_finished(graph)) {
shown_commit_line = graph_next_line(graph, &msgbuf);
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+ graph->revs->diffopt.file);
if (!shown_commit_line)
- putchar('\n');
+ putc('\n', graph->revs->diffopt.file);
strbuf_setlen(&msgbuf, 0);
}
@@ -1210,7 +1211,7 @@ void graph_show_oneline(struct git_graph *graph)
return;
graph_next_line(graph, &msgbuf);
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
strbuf_release(&msgbuf);
}
@@ -1222,7 +1223,7 @@ void graph_show_padding(struct git_graph *graph)
return;
graph_padding_line(graph, &msgbuf);
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
strbuf_release(&msgbuf);
}
@@ -1239,12 +1240,13 @@ int graph_show_remainder(struct git_graph *graph)
for (;;) {
graph_next_line(graph, &msgbuf);
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+ graph->revs->diffopt.file);
strbuf_setlen(&msgbuf, 0);
shown = 1;
if (!graph_is_commit_finished(graph))
- putchar('\n');
+ putc('\n', graph->revs->diffopt.file);
else
break;
}
@@ -1259,7 +1261,8 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
char *p;
if (!graph) {
- fwrite(sb->buf, sizeof(char), sb->len, stdout);
+ fwrite(sb->buf, sizeof(char), sb->len,
+ graph->revs->diffopt.file);
return;
}
@@ -1277,7 +1280,7 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
} else {
len = (sb->buf + sb->len) - p;
}
- fwrite(p, sizeof(char), len, stdout);
+ fwrite(p, sizeof(char), len, graph->revs->diffopt.file);
if (next_p && *next_p != '\0')
graph_show_oneline(graph);
p = next_p;
@@ -1297,7 +1300,8 @@ void graph_show_commit_msg(struct git_graph *graph,
* CMIT_FMT_USERFORMAT are already missing a terminating
* newline. All of the other formats should have it.
*/
- fwrite(sb->buf, sizeof(char), sb->len, stdout);
+ fwrite(sb->buf, sizeof(char), sb->len,
+ graph->revs->diffopt.file);
return;
}
@@ -1318,7 +1322,7 @@ void graph_show_commit_msg(struct git_graph *graph,
* new line.
*/
if (!newline_terminated)
- putchar('\n');
+ putc('\n', graph->revs->diffopt.file);
graph_show_remainder(graph);
@@ -1326,6 +1330,6 @@ void graph_show_commit_msg(struct git_graph *graph,
* If sb ends with a newline, our output should too.
*/
if (newline_terminated)
- putchar('\n');
+ putc('\n', graph->revs->diffopt.file);
}
}
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 6/9] shortlog: support outputting to streams other than stdout
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
` (4 preceding siblings ...)
2016-06-21 10:35 ` [PATCH v3 5/9] graph: respect the diffopt.file setting Johannes Schindelin
@ 2016-06-21 10:35 ` Johannes Schindelin
2016-06-21 10:35 ` [PATCH v3 7/9] format-patch: explicitly switch off color when writing to files Johannes Schindelin
` (4 subsequent siblings)
10 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
This will be needed to avoid freopen() in `git format-patch`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/shortlog.c | 13 ++++++++-----
shortlog.h | 1 +
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index bfc082e..39d74fe 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,6 +229,7 @@ void shortlog_init(struct shortlog *log)
log->wrap = DEFAULT_WRAPLEN;
log->in1 = DEFAULT_INDENT1;
log->in2 = DEFAULT_INDENT2;
+ log->file = stdout;
}
int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -310,22 +311,24 @@ void shortlog_output(struct shortlog *log)
for (i = 0; i < log->list.nr; i++) {
const struct string_list_item *item = &log->list.items[i];
if (log->summary) {
- printf("%6d\t%s\n", (int)UTIL_TO_INT(item), item->string);
+ fprintf(log->file, "%6d\t%s\n",
+ (int)UTIL_TO_INT(item), item->string);
} else {
struct string_list *onelines = item->util;
- printf("%s (%d):\n", item->string, onelines->nr);
+ fprintf(log->file, "%s (%d):\n",
+ item->string, onelines->nr);
for (j = onelines->nr - 1; j >= 0; j--) {
const char *msg = onelines->items[j].string;
if (log->wrap_lines) {
strbuf_reset(&sb);
add_wrapped_shortlog_msg(&sb, msg, log);
- fwrite(sb.buf, sb.len, 1, stdout);
+ fwrite(sb.buf, sb.len, 1, log->file);
}
else
- printf(" %s\n", msg);
+ fprintf(log->file, " %s\n", msg);
}
- putchar('\n');
+ putc('\n', log->file);
onelines->strdup_strings = 1;
string_list_clear(onelines, 0);
free(onelines);
diff --git a/shortlog.h b/shortlog.h
index de4f86f..5a326c6 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -17,6 +17,7 @@ struct shortlog {
char *common_repo_prefix;
int email;
struct string_list mailmap;
+ FILE *file;
};
void shortlog_init(struct shortlog *log);
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 7/9] format-patch: explicitly switch off color when writing to files
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
` (5 preceding siblings ...)
2016-06-21 10:35 ` [PATCH v3 6/9] shortlog: support outputting to streams other than stdout Johannes Schindelin
@ 2016-06-21 10:35 ` Johannes Schindelin
2016-06-21 10:35 ` [PATCH v3 8/9] format-patch: avoid freopen() Johannes Schindelin
` (3 subsequent siblings)
10 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
We rely on the auto-detection ("is stdout a terminal?") to determine
whether to use color in the output of format-patch or not. That happens
to work because we freopen() stdout when redirecting the output to files.
However, we are about to fix that work-around, in which case the
auto-detection has no chance to guess whether to use color or not.
But then, we do not need to guess to begin with. We know that we do not
want to use ANSI color sequences in the output files. So let's just be
explicit about our wishes.
It might be argued that we should only do this when the variable
git_use_color_default still has its default value, GIT_COLOR_AUTO.
As of 7787570c (format-patch: ignore ui.color, 2011-09-13) we
do not allow the ui.color setting to affect format-patch's output,
therefore this will always be the case.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/log.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..abd889b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1569,6 +1569,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
setup_pager();
if (output_directory) {
+ rev.diffopt.use_color = 0;
if (use_stdout)
die(_("standard output, or directory, which one?"));
if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 8/9] format-patch: avoid freopen()
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
` (6 preceding siblings ...)
2016-06-21 10:35 ` [PATCH v3 7/9] format-patch: explicitly switch off color when writing to files Johannes Schindelin
@ 2016-06-21 10:35 ` Johannes Schindelin
2016-06-21 10:35 ` [PATCH v3 9/9] format-patch: use stdout directly Johannes Schindelin
` (2 subsequent siblings)
10 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
We just taught the relevant functions to respect the diffopt.file field,
to allow writing somewhere else than stdout. Let's make use of it.
Technically, we do not need to avoid that call in a builtin: we assume
that builtins (as opposed to library functions) are stand-alone programs
that may do with their (global) state. Yet, we want to be able to reuse
that code in properly lib-ified code, e.g. when converting scripts into
builtins.
Further, while we did not *have* to touch the cmd_show() and cmd_cherry()
code paths (because they do not want to write anywhere but stdout as of
yet), it just makes sense to be consistent, making it easier and safer to
move the code later.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/log.c | 64 ++++++++++++++++++++++++++++++-----------------------------
1 file changed, 33 insertions(+), 31 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index abd889b..8dcf205 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -236,7 +236,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
if (rev->commit_format != CMIT_FMT_ONELINE)
putchar(rev->diffopt.line_termination);
}
- printf(_("Final output: %d %s\n"), nr, stage);
+ fprintf(rev->diffopt.file, _("Final output: %d %s\n"), nr, stage);
}
static struct itimerval early_output_timer;
@@ -445,7 +445,7 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
pp.fmt = rev->commit_format;
pp.date_mode = rev->date_mode;
pp_user_info(&pp, "Tagger", &out, buf, get_log_output_encoding());
- printf("%s", out.buf);
+ fprintf(rev->diffopt.file, "%s", out.buf);
strbuf_release(&out);
}
@@ -456,7 +456,7 @@ static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, con
char *buf;
unsigned long size;
- fflush(stdout);
+ fflush(rev->diffopt.file);
if (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
return stream_blob_to_fd(1, sha1, NULL, 0);
@@ -496,7 +496,7 @@ static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
}
if (offset < size)
- fwrite(buf + offset, size - offset, 1, stdout);
+ fwrite(buf + offset, size - offset, 1, rev->diffopt.file);
free(buf);
return 0;
}
@@ -505,7 +505,8 @@ static int show_tree_object(const unsigned char *sha1,
struct strbuf *base,
const char *pathname, unsigned mode, int stage, void *context)
{
- printf("%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
+ FILE *file = context;
+ fprintf(file, "%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
return 0;
}
@@ -565,7 +566,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
if (rev.shown_one)
putchar('\n');
- printf("%stag %s%s\n",
+ fprintf(rev.diffopt.file, "%stag %s%s\n",
diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
t->tag,
diff_get_color_opt(&rev.diffopt, DIFF_RESET));
@@ -584,12 +585,12 @@ int cmd_show(int argc, const char **argv, const char *prefix)
case OBJ_TREE:
if (rev.shown_one)
putchar('\n');
- printf("%stree %s%s\n\n",
+ fprintf(rev.diffopt.file, "%stree %s%s\n\n",
diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
name,
diff_get_color_opt(&rev.diffopt, DIFF_RESET));
read_tree_recursive((struct tree *)o, "", 0, 0, &match_all,
- show_tree_object, NULL);
+ show_tree_object, rev.diffopt.file);
rev.shown_one = 1;
break;
case OBJ_COMMIT:
@@ -799,7 +800,7 @@ static FILE *realstdout = NULL;
static const char *output_directory = NULL;
static int outdir_offset;
-static int reopen_stdout(struct commit *commit, const char *subject,
+static int open_next_file(struct commit *commit, const char *subject,
struct rev_info *rev, int quiet)
{
struct strbuf filename = STRBUF_INIT;
@@ -823,7 +824,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
if (!quiet)
fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
- if (freopen(filename.buf, "w", stdout) == NULL)
+ if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
return error(_("Cannot open patch file %s"), filename.buf);
strbuf_release(&filename);
@@ -882,15 +883,15 @@ static void gen_message_id(struct rev_info *info, char *base)
info->message_id = strbuf_detach(&buf, NULL);
}
-static void print_signature(void)
+static void print_signature(FILE *file)
{
if (!signature || !*signature)
return;
- printf("-- \n%s", signature);
+ fprintf(file, "-- \n%s", signature);
if (signature[strlen(signature)-1] != '\n')
- putchar('\n');
- putchar('\n');
+ putc('\n', file);
+ putc('\n', file);
}
static void add_branch_description(struct strbuf *buf, const char *branch_name)
@@ -959,7 +960,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
committer = git_committer_info(0);
if (!use_stdout &&
- reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
+ open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
return;
log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
@@ -982,7 +983,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
pp_remainder(&pp, &msg, &sb, 0);
add_branch_description(&sb, branch_name);
- printf("%s\n", sb.buf);
+ fprintf(rev->diffopt.file, "%s\n", sb.buf);
strbuf_release(&sb);
@@ -991,6 +992,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
log.wrap = 72;
log.in1 = 2;
log.in2 = 4;
+ log.file = rev->diffopt.file;
for (i = 0; i < nr; i++)
shortlog_add_commit(&log, list[i]);
@@ -1013,8 +1015,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
diffcore_std(&opts);
diff_flush(&opts);
- printf("\n");
- print_signature();
+ fprintf(rev->diffopt.file, "\n");
+ print_signature(rev->diffopt.file);
}
static const char *clean_message_id(const char *msg_id)
@@ -1324,7 +1326,7 @@ static void prepare_bases(struct base_tree_info *bases,
}
}
-static void print_bases(struct base_tree_info *bases)
+static void print_bases(struct base_tree_info *bases, FILE *file)
{
int i;
@@ -1333,11 +1335,11 @@ static void print_bases(struct base_tree_info *bases)
return;
/* Show the base commit */
- printf("base-commit: %s\n", oid_to_hex(&bases->base_commit));
+ fprintf(file, "base-commit: %s\n", oid_to_hex(&bases->base_commit));
/* Show the prerequisite patches */
for (i = bases->nr_patch_id - 1; i >= 0; i--)
- printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
+ fprintf(file, "prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
free(bases->patch_id);
bases->nr_patch_id = 0;
@@ -1694,7 +1696,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
gen_message_id(&rev, "cover");
make_cover_letter(&rev, use_stdout,
origin, nr, list, branch_name, quiet);
- print_bases(&bases);
+ print_bases(&bases, rev.diffopt.file);
total++;
start_number--;
}
@@ -1740,7 +1742,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
}
if (!use_stdout &&
- reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
+ open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
die(_("Failed to create output files"));
shown = log_tree_commit(&rev, commit);
free_commit_buffer(commit);
@@ -1755,15 +1757,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
rev.shown_one = 0;
if (shown) {
if (rev.mime_boundary)
- printf("\n--%s%s--\n\n\n",
+ fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
mime_boundary_leader,
rev.mime_boundary);
else
- print_signature();
- print_bases(&bases);
+ print_signature(rev.diffopt.file);
+ print_bases(&bases, rev.diffopt.file);
}
if (!use_stdout)
- fclose(stdout);
+ fclose(rev.diffopt.file);
}
free(list);
free(branch_name);
@@ -1795,15 +1797,15 @@ static const char * const cherry_usage[] = {
};
static void print_commit(char sign, struct commit *commit, int verbose,
- int abbrev)
+ int abbrev, FILE *file)
{
if (!verbose) {
- printf("%c %s\n", sign,
+ fprintf(file, "%c %s\n", sign,
find_unique_abbrev(commit->object.oid.hash, abbrev));
} else {
struct strbuf buf = STRBUF_INIT;
pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
- printf("%c %s %s\n", sign,
+ fprintf(file, "%c %s %s\n", sign,
find_unique_abbrev(commit->object.oid.hash, abbrev),
buf.buf);
strbuf_release(&buf);
@@ -1884,7 +1886,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
commit = list->item;
if (has_commit_patch_id(commit, &ids))
sign = '-';
- print_commit(sign, commit, verbose, abbrev);
+ print_commit(sign, commit, verbose, abbrev, revs.diffopt.file);
list = list->next;
}
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v3 9/9] format-patch: use stdout directly
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
` (7 preceding siblings ...)
2016-06-21 10:35 ` [PATCH v3 8/9] format-patch: avoid freopen() Johannes Schindelin
@ 2016-06-21 10:35 ` Johannes Schindelin
2016-06-21 13:47 ` [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field Paul Tan
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
10 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 10:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
Earlier, we freopen()ed stdout in order to write patches to files.
That forced us to duplicate stdout (naming it "realstdout") because we
*still* wanted to be able to report the file names.
As we do not abuse stdout that way anymore, we no longer need to
duplicate stdout, either.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/log.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 8dcf205..2bfcc43 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -796,7 +796,6 @@ static int git_format_config(const char *var, const char *value, void *cb)
return git_log_config(var, value, cb);
}
-static FILE *realstdout = NULL;
static const char *output_directory = NULL;
static int outdir_offset;
@@ -822,7 +821,7 @@ static int open_next_file(struct commit *commit, const char *subject,
fmt_output_subject(&filename, subject, rev);
if (!quiet)
- fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
+ printf("%s\n", filename.buf + outdir_offset);
if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
return error(_("Cannot open patch file %s"), filename.buf);
@@ -1629,9 +1628,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
get_patch_ids(&rev, &ids);
}
- if (!use_stdout)
- realstdout = xfdopen(xdup(1), "w");
-
if (prepare_revision_walk(&rev))
die(_("revision walk setup failed"));
rev.boundary = 1;
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
` (8 preceding siblings ...)
2016-06-21 10:35 ` [PATCH v3 9/9] format-patch: use stdout directly Johannes Schindelin
@ 2016-06-21 13:47 ` Paul Tan
2016-06-21 14:12 ` Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
10 siblings, 1 reply; 67+ messages in thread
From: Paul Tan @ 2016-06-21 13:47 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano, Eric Sunshine
Hi Johannes,
On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> - this uncovered a problem with builtin am, where it asked the diff
> machinery to close the file stream, but actually called the log_tree
> machinery (which might mean that this patch series inadvertently fixes
> a bug where `git am --rebasing` would write the commit message to
> stdout instead of the `patch` file when erroring out)
Please correct me if I'm wrong: looking at log-tree.c, the commit
message will not be printed when no_commit_id = 1, isn't it? This is
because we do not hit the code paths that write to stdout since
show_log() is not called.
Also, the return value of log_tree_commit() is actually a boolean
value, not an error status value, isn't it?
> This last point is a bigger issue, actually. There seem to be quite a
> few function calls in builtin/am.c whose return values that might
> indicate errors are flatly ignored. I see two calls to run_diff_index()
> whose return value then goes poof unchecked,
Thanks, future-proofing the builtin/am.c code is good, in case
run_diff_index() is updated to not call exit(128) on error in the
future.
> and several calls to
> write_state_text() and write_state_bool() with the same issue.
These functions will die() on error, so checking their error code is
not really useful. I'm not opposed to changing them to use the
write_file_gently() version though, although I don't see a need to
unless builtin/am.c is being libified.
> And I did
> not even try to review the code to that end, all I wanted was to verify
> that builtin am only has the close_file issue once (it does use it a
> second time, but that one is okay because it then calls
> run_diff_index(), i.e. the diff machinery).
>
> I am embarrassed to admit that these builtin am problems demonstrate
> that I, as a mentor of the builtin am project, failed to help make the
> patches as good as I expected myself to do.
Sorry to disappoint you :-(
Regards,
Paul
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field
2016-06-21 13:47 ` [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field Paul Tan
@ 2016-06-21 14:12 ` Johannes Schindelin
2016-06-22 9:23 ` Paul Tan
0 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-21 14:12 UTC (permalink / raw)
To: Paul Tan; +Cc: Git List, Junio C Hamano, Eric Sunshine
Hi Paul,
On Tue, 21 Jun 2016, Paul Tan wrote:
> On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > - this uncovered a problem with builtin am, where it asked the diff
> > machinery to close the file stream, but actually called the log_tree
> > machinery (which might mean that this patch series inadvertently fixes
> > a bug where `git am --rebasing` would write the commit message to
> > stdout instead of the `patch` file when erroring out)
>
> Please correct me if I'm wrong: looking at log-tree.c, the commit
> message will not be printed when no_commit_id = 1, isn't it?
> This is because we do not hit the code paths that write to stdout since
> show_log() is not called.
Why does builtin/am.c use log_tree_commit(), then? Why not simply run
things through the diff machinery?
> Also, the return value of log_tree_commit() is actually a boolean
> value, not an error status value, isn't it?
It is not really a boolean, no. Sure, at the moment, it happens to return
either 0 or 1. You can figure that out by following the call paths all the
way to do_diff_combined() or line_log_print().
The key words are: at the moment.
We do find more and more places where library functions call die() in case
of errors, and it hurts us. Badly. That is why I, among others, try to
remedy the situation by converting these calls to "return error()"
statements.
The log_tree functions are prepared for that: they return non-negative
values in case of success.
The callers are not really prepared for that, hence my complaints.
> > This last point is a bigger issue, actually. There seem to be quite a
> > few function calls in builtin/am.c whose return values that might
> > indicate errors are flatly ignored. I see two calls to run_diff_index()
> > whose return value then goes poof unchecked,
>
> Thanks, future-proofing the builtin/am.c code is good, in case
> run_diff_index() is updated to not call exit(128) on error in the
> future.
And run_diff_cache(). And read_ref_at(). And rerere(). And
setup_revisions(). And get_sha1().
> > and several calls to write_state_text() and write_state_bool() with
> > the same issue.
>
> These functions will die() on error
Indeed. And I do not think that is a good practice.
> > And I did not even try to review the code to that end, all I wanted
> > was to verify that builtin am only has the close_file issue once (it
> > does use it a second time, but that one is okay because it then calls
> > run_diff_index(), i.e. the diff machinery).
> >
> > I am embarrassed to admit that these builtin am problems demonstrate
> > that I, as a mentor of the builtin am project, failed to help make the
> > patches as good as I expected myself to do.
>
> Sorry to disappoint you :-(
You misunderstood. I am not disappointed in you. *I* did a lousy job. Not
only mentoring, but I also obviously failed to make things fun enough for
you.
My apologies,
Dscho
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field
2016-06-21 14:12 ` Johannes Schindelin
@ 2016-06-22 9:23 ` Paul Tan
0 siblings, 0 replies; 67+ messages in thread
From: Paul Tan @ 2016-06-22 9:23 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Git List, Junio C Hamano, Eric Sunshine
Hi Johannes,
On Tue, Jun 21, 2016 at 10:12 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Paul,
>
> On Tue, 21 Jun 2016, Paul Tan wrote:
>
>> On Tue, Jun 21, 2016 at 6:34 PM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>> > - this uncovered a problem with builtin am, where it asked the diff
>> > machinery to close the file stream, but actually called the log_tree
>> > machinery (which might mean that this patch series inadvertently fixes
>> > a bug where `git am --rebasing` would write the commit message to
>> > stdout instead of the `patch` file when erroring out)
>>
>> Please correct me if I'm wrong: looking at log-tree.c, the commit
>> message will not be printed when no_commit_id = 1, isn't it?
>> This is because we do not hit the code paths that write to stdout since
>> show_log() is not called.
>
> Why does builtin/am.c use log_tree_commit(), then? Why not simply run
> things through the diff machinery?
It's because git-am.sh called "git diff-tree", which in turn calls
log_tree_commit(), so to be safe I used the same function to ensure
that there were no unintended behavioral changes. e.g. what happened
with 3ecc704 (am --skip/--abort: merge HEAD/ORIG_HEAD tree into index,
2015-08-19)
Of course, it may be true that the diff machinery should be called
directly, although the code is quite involved so I can't really tell
the impact the change will have.
Thanks,
Paul
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v4 00/10] Let log-tree and friends respect diffopt's `file` field
2016-06-21 10:34 ` [PATCH v3 0/9] " Johannes Schindelin
` (9 preceding siblings ...)
2016-06-21 13:47 ` [PATCH v3 0/9] Let log-tree and friends respect diffopt's `file` field Paul Tan
@ 2016-06-22 15:01 ` Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute Johannes Schindelin
` (9 more replies)
10 siblings, 10 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
The idea is to allow callers to redirect log-tree's output to a file
without having to freopen() stdout (which would modify global state,
a big no-no-no for library functions).
I reviewed log-tree.c, graph.c, line-log.c, builtin/shortlog.c and
builtin/log.c line by line to ensure that all calls that assumed stdout
previously now use the `file` field instead, of course. I would
welcome additional eyes to go over the code to confirm that I did not
miss anything.
This patch series ends up removing the freopen() call in the
format-patch command, but that is only a by-product. The ulterior motive
behind this series is to allow the sequencer to write a `patch` file as
part of my endeavor to move large chunks of rebase -i into a builtin.
In contrast to the previous iteration of this patch series,
- the use_color = 0 setting was made contingent on use_color != ALWAYS
- close_file = 1 was made to work in more circumstances, most notably
when calling log_commit_tree() (and in builtin/log.c, where this
function is called in a loop)
- the changes to builtin/am.c were backed out completely (this is a can
of worms I am not prepared to open for now)
- I also taught shortlog to respect the --output=<file> option, because
it was so easy to do
- I added a test case to ensure that `log --output=<file>` works
Johannes Schindelin (10):
Prepare log/log-tree to reuse the diffopt.close_file attribute
log-tree: respect diffopt's configured output file stream
line-log: respect diffopt's configured output file stream
graph: respect the diffopt.file setting
shortlog: support outputting to streams other than stdout
format-patch: explicitly switch off color when writing to files
format-patch: avoid freopen()
format-patch: use stdout directly
shortlog: respect the --output=<file> setting
Ensure that log respects --output=<file>
builtin/log.c | 87 +++++++++++++++++++++++++++++------------------------
builtin/shortlog.c | 15 ++++++---
graph.c | 30 ++++++++++--------
line-log.c | 34 ++++++++++-----------
log-tree.c | 69 ++++++++++++++++++++++--------------------
shortlog.h | 1 +
t/t4201-shortlog.sh | 6 ++++
t/t4211-line-log.sh | 7 +++++
8 files changed, 142 insertions(+), 107 deletions(-)
Published-As: https://github.com/dscho/git/releases/tag/diffopt.file-v4
Interdiff vs v3:
diff --git a/builtin/am.c b/builtin/am.c
index 47d78aa..3dfe70b 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1433,16 +1433,12 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
/**
* Writes `commit` as a patch to the state directory's "patch" file.
*/
-static int write_commit_patch(const struct am_state *state, struct commit *commit)
+static void write_commit_patch(const struct am_state *state, struct commit *commit)
{
struct rev_info rev_info;
FILE *fp;
- int res;
- fp = fopen(am_path(state, "patch"), "w");
- if (!fp)
- return error(_("Could not open %s for writing"),
- am_path(state, "patch"));
+ fp = xfopen(am_path(state, "patch"), "w");
init_revisions(&rev_info, NULL);
rev_info.diff = 1;
rev_info.abbrev = 0;
@@ -1454,11 +1450,10 @@ 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);
- res = log_tree_commit(&rev_info, commit);
- fclose(fp);
- return res;
+ log_tree_commit(&rev_info, commit);
}
/**
@@ -1506,14 +1501,13 @@ static int parse_mail_rebase(struct am_state *state, const char *mail)
unsigned char commit_sha1[GIT_SHA1_RAWSZ];
if (get_mail_commit_sha1(commit_sha1, mail) < 0)
- return error(_("could not parse %s"), mail);
+ die(_("could not parse %s"), mail);
commit = lookup_commit_or_die(commit_sha1, mail);
get_commit_info(state, commit);
- if (write_commit_patch(state, commit) < 0)
- return -1;
+ write_commit_patch(state, commit);
hashcpy(state->orig_commit, commit_sha1);
write_state_text(state, "original-commit", sha1_to_hex(commit_sha1));
diff --git a/builtin/log.c b/builtin/log.c
index 2bfcc43..2a42216 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
static void log_show_early(struct rev_info *revs, struct commit_list *list)
{
- int i = revs->early_output;
+ int i = revs->early_output, close_file = revs->diffopt.close_file;
int show_header = 1;
+ revs->diffopt.close_file = 0;
sort_in_topological_order(&list, revs->sort_order);
while (list && i) {
struct commit *commit = list->item;
@@ -262,14 +263,19 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
case commit_ignore:
break;
case commit_error:
+ if (close_file)
+ fclose(revs->diffopt.file);
return;
}
list = list->next;
}
/* Did we already get enough commits for the early output? */
- if (!i)
+ if (!i) {
+ if (close_file)
+ fclose(revs->diffopt.file);
return;
+ }
/*
* ..if no, then repeat it twice a second until we
@@ -331,7 +337,7 @@ static int cmd_log_walk(struct rev_info *rev)
{
struct commit *commit;
int saved_nrl = 0;
- int saved_dcctc = 0;
+ int saved_dcctc = 0, close_file = rev->diffopt.close_file;
if (rev->early_output)
setup_early_output(rev);
@@ -347,6 +353,7 @@ static int cmd_log_walk(struct rev_info *rev)
* and HAS_CHANGES being accumulated in rev->diffopt, so be careful to
* retain that state information if replacing rev->diffopt in this loop
*/
+ rev->diffopt.close_file = 0;
while ((commit = get_revision(rev)) != NULL) {
if (!log_tree_commit(rev, commit) && rev->max_count >= 0)
/*
@@ -367,6 +374,8 @@ static int cmd_log_walk(struct rev_info *rev)
}
rev->diffopt.degraded_cc_to_c = saved_dcctc;
rev->diffopt.needed_rename_limit = saved_nrl;
+ if (close_file)
+ fclose(rev->diffopt.file);
if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {
@@ -1570,7 +1579,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
setup_pager();
if (output_directory) {
- rev.diffopt.use_color = 0;
+ if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
+ rev.diffopt.use_color = 0;
if (use_stdout)
die(_("standard output, or directory, which one?"));
if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 39d74fe..be80547 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,7 +229,6 @@ void shortlog_init(struct shortlog *log)
log->wrap = DEFAULT_WRAPLEN;
log->in1 = DEFAULT_INDENT1;
log->in2 = DEFAULT_INDENT2;
- log->file = stdout;
}
int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -277,6 +276,7 @@ parse_done:
log.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;
log.abbrev = rev.abbrev;
+ log.file = rev.diffopt.file;
/* assume HEAD if from a tty */
if (!nongit && !rev.pending.nr && isatty(0))
@@ -290,6 +290,8 @@ parse_done:
get_from_rev(&rev, &log);
shortlog_output(&log);
+ if (log.file != stdout)
+ fclose(log.file);
return 0;
}
diff --git a/log-tree.c b/log-tree.c
index 530297d..cf24027 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -862,14 +862,12 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
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()");
+ int shown, close_file = opt->diffopt.close_file;
log.commit = commit;
log.parent = NULL;
opt->loginfo = &log;
+ opt->diffopt.close_file = 0;
if (opt->line_level_traverse)
return line_log_print(opt, commit);
@@ -886,5 +884,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
opt->loginfo = NULL;
maybe_flush_or_die(opt->diffopt.file, "stdout");
+ if (close_file)
+ fclose(opt->diffopt.file);
return shown;
}
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index a977365..bd699e1 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -184,4 +184,10 @@ test_expect_success 'shortlog with revision pseudo options' '
git shortlog --exclude=refs/heads/m* --all
'
+test_expect_success 'shortlog with --output=<file>' '
+ git shortlog --output=shortlog master >output &&
+ test ! -s output &&
+ test_line_count = 7 shortlog
+'
+
test_done
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 4451127..9d87777 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -99,4 +99,11 @@ test_expect_success '-L with --first-parent and a merge' '
git log --first-parent -L 1,1:b.c
'
+test_expect_success '-L with --output' '
+ git checkout parallel-change &&
+ git log --output=log -L :main:b.c >output &&
+ test ! -s output &&
+ test_line_count = 70 log
+'
+
test_done
--
2.9.0.118.g0e1a633
base-commit: ab7797dbe95fff38d9265869ea367020046db118
^ permalink raw reply [flat|nested] 67+ messages in thread* [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
@ 2016-06-22 15:01 ` Johannes Schindelin
2016-06-24 20:56 ` Junio C Hamano
2016-06-22 15:01 ` [PATCH v4 02/10] log-tree: respect diffopt's configured output file stream Johannes Schindelin
` (8 subsequent siblings)
9 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
We are about to teach the log-tree machinery to reuse the diffopt.file
field to output to a file stream other than stdout, in line with the
diff machinery already writing to diffopt.file.
However, we might want to write something after the diff in
log_tree_commit() (e.g. with the --show-linear-break option), therefore
we must not let the diff machinery close the file (as per
diffopt.close_file.
This means that log_tree_commit() itself must override the
diffopt.close_file flag and close the file, and if log_tree_commit() is
called in a loop, the caller is responsible to do the same.
Note: format-patch has an `--output-directory` option. Due to the fact
that format-patch's options are parsed first, and that the parse-options
machinery accepts uniquely abbreviated options, the diff options
`--output` (and `-o`) are shadowed. Therefore close_file is not set to 1
so that cmd_format_patch() does *not* need to handle the close_file flag
differently, even if it calls log_tree_commit() in a loop.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/log.c | 15 ++++++++++++---
log-tree.c | 5 ++++-
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 099f4f7..27bc88d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
static void log_show_early(struct rev_info *revs, struct commit_list *list)
{
- int i = revs->early_output;
+ int i = revs->early_output, close_file = revs->diffopt.close_file;
int show_header = 1;
+ revs->diffopt.close_file = 0;
sort_in_topological_order(&list, revs->sort_order);
while (list && i) {
struct commit *commit = list->item;
@@ -262,14 +263,19 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
case commit_ignore:
break;
case commit_error:
+ if (close_file)
+ fclose(revs->diffopt.file);
return;
}
list = list->next;
}
/* Did we already get enough commits for the early output? */
- if (!i)
+ if (!i) {
+ if (close_file)
+ fclose(revs->diffopt.file);
return;
+ }
/*
* ..if no, then repeat it twice a second until we
@@ -331,7 +337,7 @@ static int cmd_log_walk(struct rev_info *rev)
{
struct commit *commit;
int saved_nrl = 0;
- int saved_dcctc = 0;
+ int saved_dcctc = 0, close_file = rev->diffopt.close_file;
if (rev->early_output)
setup_early_output(rev);
@@ -347,6 +353,7 @@ static int cmd_log_walk(struct rev_info *rev)
* and HAS_CHANGES being accumulated in rev->diffopt, so be careful to
* retain that state information if replacing rev->diffopt in this loop
*/
+ rev->diffopt.close_file = 0;
while ((commit = get_revision(rev)) != NULL) {
if (!log_tree_commit(rev, commit) && rev->max_count >= 0)
/*
@@ -367,6 +374,8 @@ static int cmd_log_walk(struct rev_info *rev)
}
rev->diffopt.degraded_cc_to_c = saved_dcctc;
rev->diffopt.needed_rename_limit = saved_nrl;
+ if (close_file)
+ fclose(rev->diffopt.file);
if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {
diff --git a/log-tree.c b/log-tree.c
index 78a5381..456d7e3 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -862,11 +862,12 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
int log_tree_commit(struct rev_info *opt, struct commit *commit)
{
struct log_info log;
- int shown;
+ int shown, close_file = opt->diffopt.close_file;
log.commit = commit;
log.parent = NULL;
opt->loginfo = &log;
+ opt->diffopt.close_file = 0;
if (opt->line_level_traverse)
return line_log_print(opt, commit);
@@ -883,5 +884,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
printf("\n%s\n", opt->break_bar);
opt->loginfo = NULL;
maybe_flush_or_die(stdout, "stdout");
+ if (close_file)
+ fclose(opt->diffopt.file);
return shown;
}
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute
2016-06-22 15:01 ` [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute Johannes Schindelin
@ 2016-06-24 20:56 ` Junio C Hamano
2016-06-26 6:56 ` Johannes Schindelin
0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2016-06-24 20:56 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> We are about to teach the log-tree machinery to reuse the diffopt.file
> field to output to a file stream other than stdout, in line with the
> diff machinery already writing to diffopt.file.
>
> However, we might want to write something after the diff in
> log_tree_commit() (e.g. with the --show-linear-break option), therefore
> we must not let the diff machinery close the file (as per
> diffopt.close_file.
>
> This means that log_tree_commit() itself must override the
> diffopt.close_file flag and close the file, and if log_tree_commit() is
> called in a loop, the caller is responsible to do the same.
Makes sense.
> Note: format-patch has an `--output-directory` option. Due to the fact
> that format-patch's options are parsed first, and that the parse-options
> machinery accepts uniquely abbreviated options, the diff options
> `--output` (and `-o`) are shadowed. Therefore close_file is not set to 1
> so that cmd_format_patch() does *not* need to handle the close_file flag
> differently, even if it calls log_tree_commit() in a loop.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/log.c | 15 ++++++++++++---
> log-tree.c | 5 ++++-
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 099f4f7..27bc88d 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
>
> static void log_show_early(struct rev_info *revs, struct commit_list *list)
> {
> - int i = revs->early_output;
> + int i = revs->early_output, close_file = revs->diffopt.close_file;
Probably not worth a reroll, but I find this kind of thing easier to
read as two separate definitions.
> int show_header = 1;
And this was separate from "int i = ...;" for the same reason. It
is OK to write "int i, j, k;" but "show_header" is something that
keeps track of the more important state during the control flow and
it is easier to read if we make it stand out. close_file falls into
the same category, I would think.
> case commit_error:
> + if (close_file)
> + fclose(revs->diffopt.file);
I wondered if we want to also clear, i.e. revs->diffopt.file = NULL,
but I do not think .close_file does that either, so this is good.
Thanks.
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute
2016-06-24 20:56 ` Junio C Hamano
@ 2016-06-26 6:56 ` Johannes Schindelin
0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-26 6:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine
Hi Junio,
On Fri, 24 Jun 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > diff --git a/builtin/log.c b/builtin/log.c
> > index 099f4f7..27bc88d 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -243,9 +243,10 @@ static struct itimerval early_output_timer;
> >
> > static void log_show_early(struct rev_info *revs, struct commit_list *list)
> > {
> > - int i = revs->early_output;
> > + int i = revs->early_output, close_file = revs->diffopt.close_file;
>
> Probably not worth a reroll, but I find this kind of thing easier to
> read as two separate definitions.
>
> > int show_header = 1;
>
> And this was separate from "int i = ...;" for the same reason. It
> is OK to write "int i, j, k;" but "show_header" is something that
> keeps track of the more important state during the control flow and
> it is easier to read if we make it stand out. close_file falls into
> the same category, I would think.
Makes sense. I changed it locally, in case this patch series needs a
re-roll.
> > case commit_error:
> > + if (close_file)
> > + fclose(revs->diffopt.file);
>
> I wondered if we want to also clear, i.e. revs->diffopt.file = NULL,
> but I do not think .close_file does that either, so this is good.
Indeed, the current code in diff_flush() just fclose()s the stream, but
does not reset the "file" field:
https://github.com/git/git/blob/v2.9.0/diff.c#L4715-L4716
Ciao,
Dscho
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v4 02/10] log-tree: respect diffopt's configured output file stream
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute Johannes Schindelin
@ 2016-06-22 15:01 ` Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 03/10] line-log: " Johannes Schindelin
` (7 subsequent siblings)
9 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
The diff options already know how to print the output anywhere else
than stdout. The same is needed for log output in general, e.g.
when writing patches to files in `git format-patch`. Let's allow
users to use log_tree_commit() *without* changing global state via
freopen().
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
log-tree.c | 64 +++++++++++++++++++++++++++++++-------------------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 456d7e3..cf24027 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -159,12 +159,12 @@ void load_ref_decorations(int flags)
}
}
-static void show_parents(struct commit *commit, int abbrev)
+static void show_parents(struct commit *commit, int abbrev, FILE *file)
{
struct commit_list *p;
for (p = commit->parents; p ; p = p->next) {
struct commit *parent = p->item;
- printf(" %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
+ fprintf(file, " %s", find_unique_abbrev(parent->object.oid.hash, abbrev));
}
}
@@ -172,7 +172,7 @@ static void show_children(struct rev_info *opt, struct commit *commit, int abbre
{
struct commit_list *p = lookup_decoration(&opt->children, &commit->object);
for ( ; p; p = p->next) {
- printf(" %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
+ fprintf(opt->diffopt.file, " %s", find_unique_abbrev(p->item->object.oid.hash, abbrev));
}
}
@@ -286,11 +286,11 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
struct strbuf sb = STRBUF_INIT;
if (opt->show_source && commit->util)
- printf("\t%s", (char *) commit->util);
+ fprintf(opt->diffopt.file, "\t%s", (char *) commit->util);
if (!opt->show_decorations)
return;
format_decorations(&sb, commit, opt->diffopt.use_color);
- fputs(sb.buf, stdout);
+ fputs(sb.buf, opt->diffopt.file);
strbuf_release(&sb);
}
@@ -364,18 +364,18 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
subject = "Subject: ";
}
- printf("From %s Mon Sep 17 00:00:00 2001\n", name);
+ fprintf(opt->diffopt.file, "From %s Mon Sep 17 00:00:00 2001\n", name);
graph_show_oneline(opt->graph);
if (opt->message_id) {
- printf("Message-Id: <%s>\n", opt->message_id);
+ fprintf(opt->diffopt.file, "Message-Id: <%s>\n", opt->message_id);
graph_show_oneline(opt->graph);
}
if (opt->ref_message_ids && opt->ref_message_ids->nr > 0) {
int i, n;
n = opt->ref_message_ids->nr;
- printf("In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
+ fprintf(opt->diffopt.file, "In-Reply-To: <%s>\n", opt->ref_message_ids->items[n-1].string);
for (i = 0; i < n; i++)
- printf("%s<%s>\n", (i > 0 ? "\t" : "References: "),
+ fprintf(opt->diffopt.file, "%s<%s>\n", (i > 0 ? "\t" : "References: "),
opt->ref_message_ids->items[i].string);
graph_show_oneline(opt->graph);
}
@@ -432,7 +432,7 @@ static void show_sig_lines(struct rev_info *opt, int status, const char *bol)
reset = diff_get_color_opt(&opt->diffopt, DIFF_RESET);
while (*bol) {
eol = strchrnul(bol, '\n');
- printf("%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
+ fprintf(opt->diffopt.file, "%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
*eol ? "\n" : "");
graph_show_oneline(opt->graph);
bol = (*eol) ? (eol + 1) : eol;
@@ -553,17 +553,17 @@ void show_log(struct rev_info *opt)
if (!opt->graph)
put_revision_mark(opt, commit);
- fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), stdout);
+ fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit), opt->diffopt.file);
if (opt->print_parents)
- show_parents(commit, abbrev_commit);
+ show_parents(commit, abbrev_commit, opt->diffopt.file);
if (opt->children.name)
show_children(opt, commit, abbrev_commit);
show_decorations(opt, commit);
if (opt->graph && !graph_is_commit_finished(opt->graph)) {
- putchar('\n');
+ putc('\n', opt->diffopt.file);
graph_show_remainder(opt->graph);
}
- putchar(opt->diffopt.line_termination);
+ putc(opt->diffopt.line_termination, opt->diffopt.file);
return;
}
@@ -589,7 +589,7 @@ void show_log(struct rev_info *opt)
if (opt->diffopt.line_termination == '\n' &&
!opt->missing_newline)
graph_show_padding(opt->graph);
- putchar(opt->diffopt.line_termination);
+ putc(opt->diffopt.line_termination, opt->diffopt.file);
}
opt->shown_one = 1;
@@ -607,28 +607,28 @@ void show_log(struct rev_info *opt)
log_write_email_headers(opt, commit, &ctx.subject, &extra_headers,
&ctx.need_8bit_cte);
} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
- fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), stdout);
+ fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), opt->diffopt.file);
if (opt->commit_format != CMIT_FMT_ONELINE)
- fputs("commit ", stdout);
+ fputs("commit ", opt->diffopt.file);
if (!opt->graph)
put_revision_mark(opt, commit);
fputs(find_unique_abbrev(commit->object.oid.hash, abbrev_commit),
- stdout);
+ opt->diffopt.file);
if (opt->print_parents)
- show_parents(commit, abbrev_commit);
+ show_parents(commit, abbrev_commit, opt->diffopt.file);
if (opt->children.name)
show_children(opt, commit, abbrev_commit);
if (parent)
- printf(" (from %s)",
+ fprintf(opt->diffopt.file, " (from %s)",
find_unique_abbrev(parent->object.oid.hash,
abbrev_commit));
- fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), stdout);
+ fputs(diff_get_color_opt(&opt->diffopt, DIFF_RESET), opt->diffopt.file);
show_decorations(opt, commit);
if (opt->commit_format == CMIT_FMT_ONELINE) {
- putchar(' ');
+ putc(' ', opt->diffopt.file);
} else {
- putchar('\n');
+ putc('\n', opt->diffopt.file);
graph_show_oneline(opt->graph);
}
if (opt->reflog_info) {
@@ -702,7 +702,7 @@ void show_log(struct rev_info *opt)
}
if (opt->show_log_size) {
- printf("log size %i\n", (int)msgbuf.len);
+ fprintf(opt->diffopt.file, "log size %i\n", (int)msgbuf.len);
graph_show_oneline(opt->graph);
}
@@ -718,11 +718,11 @@ void show_log(struct rev_info *opt)
if (opt->graph)
graph_show_commit_msg(opt->graph, &msgbuf);
else
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len, opt->diffopt.file);
if (opt->use_terminator && !commit_format_is_empty(opt->commit_format)) {
if (!opt->missing_newline)
graph_show_padding(opt->graph);
- putchar(opt->diffopt.line_termination);
+ putc(opt->diffopt.line_termination, opt->diffopt.file);
}
strbuf_release(&msgbuf);
@@ -759,7 +759,7 @@ int log_tree_diff_flush(struct rev_info *opt)
struct strbuf *msg = NULL;
msg = opt->diffopt.output_prefix(&opt->diffopt,
opt->diffopt.output_prefix_data);
- fwrite(msg->buf, msg->len, 1, stdout);
+ fwrite(msg->buf, msg->len, 1, opt->diffopt.file);
}
/*
@@ -774,8 +774,8 @@ int log_tree_diff_flush(struct rev_info *opt)
*/
if (!opt->shown_dashes &&
(pch & opt->diffopt.output_format) == pch)
- printf("---");
- putchar('\n');
+ fprintf(opt->diffopt.file, "---");
+ putc('\n', opt->diffopt.file);
}
}
diff_flush(&opt->diffopt);
@@ -873,7 +873,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
return line_log_print(opt, commit);
if (opt->track_linear && !opt->linear && !opt->reverse_output_stage)
- printf("\n%s\n", opt->break_bar);
+ fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
shown = log_tree_diff(opt, commit, &log);
if (!shown && opt->loginfo && opt->always_show_header) {
log.parent = NULL;
@@ -881,9 +881,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
shown = 1;
}
if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
- printf("\n%s\n", opt->break_bar);
+ fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
opt->loginfo = NULL;
- maybe_flush_or_die(stdout, "stdout");
+ maybe_flush_or_die(opt->diffopt.file, "stdout");
if (close_file)
fclose(opt->diffopt.file);
return shown;
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 03/10] line-log: respect diffopt's configured output file stream
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 01/10] Prepare log/log-tree to reuse the diffopt.close_file attribute Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 02/10] log-tree: respect diffopt's configured output file stream Johannes Schindelin
@ 2016-06-22 15:01 ` Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 04/10] graph: respect the diffopt.file setting Johannes Schindelin
` (6 subsequent siblings)
9 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
The diff machinery can optionally output to a file stream other than
stdout, by overriding diffopt.file. In such a case, the rest of the
log tree machinery should also write to that stream.
Currently, there is no user of the line level log that wants to
redirect output to a file. Therefore, one might argue that it is
superfluous to support that now. However, it is better to be
consistent now, rather than to face hard-to-debug problems later.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
line-log.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/line-log.c b/line-log.c
index bbe31ed..e62a7f4 100644
--- a/line-log.c
+++ b/line-log.c
@@ -841,7 +841,7 @@ static char *get_nth_line(long line, unsigned long *ends, void *data)
static void print_line(const char *prefix, char first,
long line, unsigned long *ends, void *data,
- const char *color, const char *reset)
+ const char *color, const char *reset, FILE *file)
{
char *begin = get_nth_line(line, ends, data);
char *end = get_nth_line(line+1, ends, data);
@@ -852,14 +852,14 @@ static void print_line(const char *prefix, char first,
had_nl = 1;
}
- fputs(prefix, stdout);
- fputs(color, stdout);
- putchar(first);
- fwrite(begin, 1, end-begin, stdout);
- fputs(reset, stdout);
- putchar('\n');
+ fputs(prefix, file);
+ fputs(color, file);
+ putc(first, file);
+ fwrite(begin, 1, end-begin, file);
+ fputs(reset, file);
+ putc('\n', file);
if (!had_nl)
- fputs("\\ No newline at end of file\n", stdout);
+ fputs("\\ No newline at end of file\n", file);
}
static char *output_prefix(struct diff_options *opt)
@@ -898,12 +898,12 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
fill_line_ends(pair->one, &p_lines, &p_ends);
fill_line_ends(pair->two, &t_lines, &t_ends);
- printf("%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
- printf("%s%s--- %s%s%s\n", prefix, c_meta,
+ fprintf(opt->file, "%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
+ fprintf(opt->file, "%s%s--- %s%s%s\n", prefix, c_meta,
pair->one->sha1_valid ? "a/" : "",
pair->one->sha1_valid ? pair->one->path : "/dev/null",
c_reset);
- printf("%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
+ fprintf(opt->file, "%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
for (i = 0; i < range->ranges.nr; i++) {
long p_start, p_end;
long t_start = range->ranges.ranges[i].start;
@@ -945,7 +945,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
}
/* Now output a diff hunk for this range */
- printf("%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
+ fprintf(opt->file, "%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
prefix, c_frag,
p_start+1, p_end-p_start, t_start+1, t_end-t_start,
c_reset);
@@ -953,18 +953,18 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
int k;
for (; t_cur < diff->target.ranges[j].start; t_cur++)
print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
- c_context, c_reset);
+ c_context, c_reset, opt->file);
for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
print_line(prefix, '-', k, p_ends, pair->one->data,
- c_old, c_reset);
+ c_old, c_reset, opt->file);
for (; t_cur < diff->target.ranges[j].end && t_cur < t_end; t_cur++)
print_line(prefix, '+', t_cur, t_ends, pair->two->data,
- c_new, c_reset);
+ c_new, c_reset, opt->file);
j++;
}
for (; t_cur < t_end; t_cur++)
print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
- c_context, c_reset);
+ c_context, c_reset, opt->file);
}
free(p_ends);
@@ -977,7 +977,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
*/
static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
{
- puts(output_prefix(&rev->diffopt));
+ fprintf(rev->diffopt.file, "%s\n", output_prefix(&rev->diffopt));
while (range) {
dump_diff_hacky_one(rev, range);
range = range->next;
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 04/10] graph: respect the diffopt.file setting
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
` (2 preceding siblings ...)
2016-06-22 15:01 ` [PATCH v4 03/10] line-log: " Johannes Schindelin
@ 2016-06-22 15:01 ` Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 05/10] shortlog: support outputting to streams other than stdout Johannes Schindelin
` (5 subsequent siblings)
9 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
When the caller overrides diffopt.file (which defaults to stdout),
the diff machinery already redirects its output, and the graph display
should also write to that file.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
graph.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/graph.c b/graph.c
index 1350bdd..8ad8ba3 100644
--- a/graph.c
+++ b/graph.c
@@ -17,8 +17,8 @@
static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
/*
- * Print a strbuf to stdout. If the graph is non-NULL, all lines but the
- * first will be prefixed with the graph output.
+ * Print a strbuf. If the graph is non-NULL, all lines but the first will be
+ * prefixed with the graph output.
*
* If the strbuf ends with a newline, the output will end after this
* newline. A new graph line will not be printed after the final newline.
@@ -1193,9 +1193,10 @@ void graph_show_commit(struct git_graph *graph)
while (!shown_commit_line && !graph_is_commit_finished(graph)) {
shown_commit_line = graph_next_line(graph, &msgbuf);
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+ graph->revs->diffopt.file);
if (!shown_commit_line)
- putchar('\n');
+ putc('\n', graph->revs->diffopt.file);
strbuf_setlen(&msgbuf, 0);
}
@@ -1210,7 +1211,7 @@ void graph_show_oneline(struct git_graph *graph)
return;
graph_next_line(graph, &msgbuf);
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
strbuf_release(&msgbuf);
}
@@ -1222,7 +1223,7 @@ void graph_show_padding(struct git_graph *graph)
return;
graph_padding_line(graph, &msgbuf);
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len, graph->revs->diffopt.file);
strbuf_release(&msgbuf);
}
@@ -1239,12 +1240,13 @@ int graph_show_remainder(struct git_graph *graph)
for (;;) {
graph_next_line(graph, &msgbuf);
- fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout);
+ fwrite(msgbuf.buf, sizeof(char), msgbuf.len,
+ graph->revs->diffopt.file);
strbuf_setlen(&msgbuf, 0);
shown = 1;
if (!graph_is_commit_finished(graph))
- putchar('\n');
+ putc('\n', graph->revs->diffopt.file);
else
break;
}
@@ -1259,7 +1261,8 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
char *p;
if (!graph) {
- fwrite(sb->buf, sizeof(char), sb->len, stdout);
+ fwrite(sb->buf, sizeof(char), sb->len,
+ graph->revs->diffopt.file);
return;
}
@@ -1277,7 +1280,7 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb)
} else {
len = (sb->buf + sb->len) - p;
}
- fwrite(p, sizeof(char), len, stdout);
+ fwrite(p, sizeof(char), len, graph->revs->diffopt.file);
if (next_p && *next_p != '\0')
graph_show_oneline(graph);
p = next_p;
@@ -1297,7 +1300,8 @@ void graph_show_commit_msg(struct git_graph *graph,
* CMIT_FMT_USERFORMAT are already missing a terminating
* newline. All of the other formats should have it.
*/
- fwrite(sb->buf, sizeof(char), sb->len, stdout);
+ fwrite(sb->buf, sizeof(char), sb->len,
+ graph->revs->diffopt.file);
return;
}
@@ -1318,7 +1322,7 @@ void graph_show_commit_msg(struct git_graph *graph,
* new line.
*/
if (!newline_terminated)
- putchar('\n');
+ putc('\n', graph->revs->diffopt.file);
graph_show_remainder(graph);
@@ -1326,6 +1330,6 @@ void graph_show_commit_msg(struct git_graph *graph,
* If sb ends with a newline, our output should too.
*/
if (newline_terminated)
- putchar('\n');
+ putc('\n', graph->revs->diffopt.file);
}
}
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 05/10] shortlog: support outputting to streams other than stdout
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
` (3 preceding siblings ...)
2016-06-22 15:01 ` [PATCH v4 04/10] graph: respect the diffopt.file setting Johannes Schindelin
@ 2016-06-22 15:01 ` Johannes Schindelin
2016-06-22 15:01 ` [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files Johannes Schindelin
` (4 subsequent siblings)
9 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
This will be needed to avoid freopen() in `git format-patch`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/shortlog.c | 13 ++++++++-----
shortlog.h | 1 +
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index bfc082e..39d74fe 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,6 +229,7 @@ void shortlog_init(struct shortlog *log)
log->wrap = DEFAULT_WRAPLEN;
log->in1 = DEFAULT_INDENT1;
log->in2 = DEFAULT_INDENT2;
+ log->file = stdout;
}
int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -310,22 +311,24 @@ void shortlog_output(struct shortlog *log)
for (i = 0; i < log->list.nr; i++) {
const struct string_list_item *item = &log->list.items[i];
if (log->summary) {
- printf("%6d\t%s\n", (int)UTIL_TO_INT(item), item->string);
+ fprintf(log->file, "%6d\t%s\n",
+ (int)UTIL_TO_INT(item), item->string);
} else {
struct string_list *onelines = item->util;
- printf("%s (%d):\n", item->string, onelines->nr);
+ fprintf(log->file, "%s (%d):\n",
+ item->string, onelines->nr);
for (j = onelines->nr - 1; j >= 0; j--) {
const char *msg = onelines->items[j].string;
if (log->wrap_lines) {
strbuf_reset(&sb);
add_wrapped_shortlog_msg(&sb, msg, log);
- fwrite(sb.buf, sb.len, 1, stdout);
+ fwrite(sb.buf, sb.len, 1, log->file);
}
else
- printf(" %s\n", msg);
+ fprintf(log->file, " %s\n", msg);
}
- putchar('\n');
+ putc('\n', log->file);
onelines->strdup_strings = 1;
string_list_clear(onelines, 0);
free(onelines);
diff --git a/shortlog.h b/shortlog.h
index de4f86f..5a326c6 100644
--- a/shortlog.h
+++ b/shortlog.h
@@ -17,6 +17,7 @@ struct shortlog {
char *common_repo_prefix;
int email;
struct string_list mailmap;
+ FILE *file;
};
void shortlog_init(struct shortlog *log);
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
` (4 preceding siblings ...)
2016-06-22 15:01 ` [PATCH v4 05/10] shortlog: support outputting to streams other than stdout Johannes Schindelin
@ 2016-06-22 15:01 ` Johannes Schindelin
2016-06-24 22:01 ` Junio C Hamano
2016-06-22 15:01 ` [PATCH v4 07/10] format-patch: avoid freopen() Johannes Schindelin
` (3 subsequent siblings)
9 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
We rely on the auto-detection ("is stdout a terminal?") to determine
whether to use color in the output of format-patch or not. That happens
to work because we freopen() stdout when redirecting the output to files.
However, we are about to fix that work-around, in which case the
auto-detection has no chance to guess whether to use color or not.
But then, we do not need to guess to begin with. As argued in the commit
message of 7787570c (format-patch: ignore ui.color, 2011-09-13), we do not
allow the ui.color setting to affect format-patch's output. The only time,
therefore, that we allow color sequences to be written to the output files
is when the user specified the --color command-line option explicitly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/log.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/log.c b/builtin/log.c
index 27bc88d..5683a42 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1578,6 +1578,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
setup_pager();
if (output_directory) {
+ if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
+ rev.diffopt.use_color = 0;
if (use_stdout)
die(_("standard output, or directory, which one?"));
if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files
2016-06-22 15:01 ` [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files Johannes Schindelin
@ 2016-06-24 22:01 ` Junio C Hamano
2016-06-26 6:49 ` Johannes Schindelin
0 siblings, 1 reply; 67+ messages in thread
From: Junio C Hamano @ 2016-06-24 22:01 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> We rely on the auto-detection ("is stdout a terminal?") to determine
> whether to use color in the output of format-patch or not. That happens
> to work because we freopen() stdout when redirecting the output to files.
>
> However, we are about to fix that work-around, in which case the
> auto-detection has no chance to guess whether to use color or not.
>
> But then, we do not need to guess to begin with. As argued in the commit
> message of 7787570c (format-patch: ignore ui.color, 2011-09-13), we do not
> allow the ui.color setting to affect format-patch's output. The only time,
> therefore, that we allow color sequences to be written to the output files
> is when the user specified the --color command-line option explicitly.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
The right fix in the longer term (long after this series lands, that
is) is probably to update the world view that the codepath from
want_color_auto() to check_auto_color() has always held. In their
world view, when they are asked to make --color=auto decision, the
output always goes the standard output, and that is why they
hardcode isatty(1) to decide. The existing freopen() was a part of
that world view.
We'd need a workaround like this patch if we want to leave the
want_color_auto() as-is, and as a workaround I think this is the
least invasive one, so let's queue it as-is.
If the codepaths that use diffopt.file (not just this one that is
about output directory hence known to be writing to a file, but all
the log/diff family of commands after this series up to 5/10 has
been applied) have a way to tell want_color_auto() that the output
is going to fileno(diffopt.file), and have check_auto_color() use
that fd instead of the hardcoded 1, the problem this step is trying
to address goes away, and I think that would be the longer-term fix.
Thanks.
> builtin/log.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 27bc88d..5683a42 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1578,6 +1578,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> setup_pager();
>
> if (output_directory) {
> + if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
> + rev.diffopt.use_color = 0;
> if (use_stdout)
> die(_("standard output, or directory, which one?"));
> if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)
^ permalink raw reply [flat|nested] 67+ messages in thread* Re: [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files
2016-06-24 22:01 ` Junio C Hamano
@ 2016-06-26 6:49 ` Johannes Schindelin
0 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-26 6:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine
Hi Junio,
On Fri, 24 Jun 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > We rely on the auto-detection ("is stdout a terminal?") to determine
> > whether to use color in the output of format-patch or not. That
> > happens to work because we freopen() stdout when redirecting the
> > output to files.
> >
> > However, we are about to fix that work-around, in which case the
> > auto-detection has no chance to guess whether to use color or not.
> >
> > But then, we do not need to guess to begin with. As argued in the
> > commit message of 7787570c (format-patch: ignore ui.color,
> > 2011-09-13), we do not allow the ui.color setting to affect
> > format-patch's output. The only time, therefore, that we allow color
> > sequences to be written to the output files is when the user specified
> > the --color command-line option explicitly.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
>
> The right fix in the longer term (long after this series lands, that
> is) is probably to update the world view that the codepath from
> want_color_auto() to check_auto_color() has always held. In their
> world view, when they are asked to make --color=auto decision, the
> output always goes the standard output, and that is why they
> hardcode isatty(1) to decide. The existing freopen() was a part of
> that world view.
I agree, and I briefly looked into this. It looks a bit more involved than
I am prepared for, what with my real goal being the rebase--helper, not
clean-ups ;-)
> We'd need a workaround like this patch if we want to leave the
> want_color_auto() as-is, and as a workaround I think this is the
> least invasive one, so let's queue it as-is.
Thanks!
Dscho
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v4 07/10] format-patch: avoid freopen()
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
` (5 preceding siblings ...)
2016-06-22 15:01 ` [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files Johannes Schindelin
@ 2016-06-22 15:01 ` Johannes Schindelin
2016-06-22 15:02 ` [PATCH v4 08/10] format-patch: use stdout directly Johannes Schindelin
` (2 subsequent siblings)
9 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:01 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
We just taught the relevant functions to respect the diffopt.file field,
to allow writing somewhere else than stdout. Let's make use of it.
Technically, we do not need to avoid that call in a builtin: we assume
that builtins (as opposed to library functions) are stand-alone programs
that may do with their (global) state. Yet, we want to be able to reuse
that code in properly lib-ified code, e.g. when converting scripts into
builtins.
Further, while we did not *have* to touch the cmd_show() and cmd_cherry()
code paths (because they do not want to write anywhere but stdout as of
yet), it just makes sense to be consistent, making it easier and safer to
move the code later.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/log.c | 64 ++++++++++++++++++++++++++++++-----------------------------
1 file changed, 33 insertions(+), 31 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 5683a42..869c23b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -236,7 +236,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
if (rev->commit_format != CMIT_FMT_ONELINE)
putchar(rev->diffopt.line_termination);
}
- printf(_("Final output: %d %s\n"), nr, stage);
+ fprintf(rev->diffopt.file, _("Final output: %d %s\n"), nr, stage);
}
static struct itimerval early_output_timer;
@@ -454,7 +454,7 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
pp.fmt = rev->commit_format;
pp.date_mode = rev->date_mode;
pp_user_info(&pp, "Tagger", &out, buf, get_log_output_encoding());
- printf("%s", out.buf);
+ fprintf(rev->diffopt.file, "%s", out.buf);
strbuf_release(&out);
}
@@ -465,7 +465,7 @@ static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, con
char *buf;
unsigned long size;
- fflush(stdout);
+ fflush(rev->diffopt.file);
if (!DIFF_OPT_TOUCHED(&rev->diffopt, ALLOW_TEXTCONV) ||
!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))
return stream_blob_to_fd(1, sha1, NULL, 0);
@@ -505,7 +505,7 @@ static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
}
if (offset < size)
- fwrite(buf + offset, size - offset, 1, stdout);
+ fwrite(buf + offset, size - offset, 1, rev->diffopt.file);
free(buf);
return 0;
}
@@ -514,7 +514,8 @@ static int show_tree_object(const unsigned char *sha1,
struct strbuf *base,
const char *pathname, unsigned mode, int stage, void *context)
{
- printf("%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
+ FILE *file = context;
+ fprintf(file, "%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
return 0;
}
@@ -574,7 +575,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
if (rev.shown_one)
putchar('\n');
- printf("%stag %s%s\n",
+ fprintf(rev.diffopt.file, "%stag %s%s\n",
diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
t->tag,
diff_get_color_opt(&rev.diffopt, DIFF_RESET));
@@ -593,12 +594,12 @@ int cmd_show(int argc, const char **argv, const char *prefix)
case OBJ_TREE:
if (rev.shown_one)
putchar('\n');
- printf("%stree %s%s\n\n",
+ fprintf(rev.diffopt.file, "%stree %s%s\n\n",
diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
name,
diff_get_color_opt(&rev.diffopt, DIFF_RESET));
read_tree_recursive((struct tree *)o, "", 0, 0, &match_all,
- show_tree_object, NULL);
+ show_tree_object, rev.diffopt.file);
rev.shown_one = 1;
break;
case OBJ_COMMIT:
@@ -808,7 +809,7 @@ static FILE *realstdout = NULL;
static const char *output_directory = NULL;
static int outdir_offset;
-static int reopen_stdout(struct commit *commit, const char *subject,
+static int open_next_file(struct commit *commit, const char *subject,
struct rev_info *rev, int quiet)
{
struct strbuf filename = STRBUF_INIT;
@@ -832,7 +833,7 @@ static int reopen_stdout(struct commit *commit, const char *subject,
if (!quiet)
fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
- if (freopen(filename.buf, "w", stdout) == NULL)
+ if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
return error(_("Cannot open patch file %s"), filename.buf);
strbuf_release(&filename);
@@ -891,15 +892,15 @@ static void gen_message_id(struct rev_info *info, char *base)
info->message_id = strbuf_detach(&buf, NULL);
}
-static void print_signature(void)
+static void print_signature(FILE *file)
{
if (!signature || !*signature)
return;
- printf("-- \n%s", signature);
+ fprintf(file, "-- \n%s", signature);
if (signature[strlen(signature)-1] != '\n')
- putchar('\n');
- putchar('\n');
+ putc('\n', file);
+ putc('\n', file);
}
static void add_branch_description(struct strbuf *buf, const char *branch_name)
@@ -968,7 +969,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
committer = git_committer_info(0);
if (!use_stdout &&
- reopen_stdout(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
+ open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
return;
log_write_email_headers(rev, head, &pp.subject, &pp.after_subject,
@@ -991,7 +992,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
pp_remainder(&pp, &msg, &sb, 0);
add_branch_description(&sb, branch_name);
- printf("%s\n", sb.buf);
+ fprintf(rev->diffopt.file, "%s\n", sb.buf);
strbuf_release(&sb);
@@ -1000,6 +1001,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
log.wrap = 72;
log.in1 = 2;
log.in2 = 4;
+ log.file = rev->diffopt.file;
for (i = 0; i < nr; i++)
shortlog_add_commit(&log, list[i]);
@@ -1022,8 +1024,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
diffcore_std(&opts);
diff_flush(&opts);
- printf("\n");
- print_signature();
+ fprintf(rev->diffopt.file, "\n");
+ print_signature(rev->diffopt.file);
}
static const char *clean_message_id(const char *msg_id)
@@ -1333,7 +1335,7 @@ static void prepare_bases(struct base_tree_info *bases,
}
}
-static void print_bases(struct base_tree_info *bases)
+static void print_bases(struct base_tree_info *bases, FILE *file)
{
int i;
@@ -1342,11 +1344,11 @@ static void print_bases(struct base_tree_info *bases)
return;
/* Show the base commit */
- printf("base-commit: %s\n", oid_to_hex(&bases->base_commit));
+ fprintf(file, "base-commit: %s\n", oid_to_hex(&bases->base_commit));
/* Show the prerequisite patches */
for (i = bases->nr_patch_id - 1; i >= 0; i--)
- printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
+ fprintf(file, "prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
free(bases->patch_id);
bases->nr_patch_id = 0;
@@ -1704,7 +1706,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
gen_message_id(&rev, "cover");
make_cover_letter(&rev, use_stdout,
origin, nr, list, branch_name, quiet);
- print_bases(&bases);
+ print_bases(&bases, rev.diffopt.file);
total++;
start_number--;
}
@@ -1750,7 +1752,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
}
if (!use_stdout &&
- reopen_stdout(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
+ open_next_file(rev.numbered_files ? NULL : commit, NULL, &rev, quiet))
die(_("Failed to create output files"));
shown = log_tree_commit(&rev, commit);
free_commit_buffer(commit);
@@ -1765,15 +1767,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
rev.shown_one = 0;
if (shown) {
if (rev.mime_boundary)
- printf("\n--%s%s--\n\n\n",
+ fprintf(rev.diffopt.file, "\n--%s%s--\n\n\n",
mime_boundary_leader,
rev.mime_boundary);
else
- print_signature();
- print_bases(&bases);
+ print_signature(rev.diffopt.file);
+ print_bases(&bases, rev.diffopt.file);
}
if (!use_stdout)
- fclose(stdout);
+ fclose(rev.diffopt.file);
}
free(list);
free(branch_name);
@@ -1805,15 +1807,15 @@ static const char * const cherry_usage[] = {
};
static void print_commit(char sign, struct commit *commit, int verbose,
- int abbrev)
+ int abbrev, FILE *file)
{
if (!verbose) {
- printf("%c %s\n", sign,
+ fprintf(file, "%c %s\n", sign,
find_unique_abbrev(commit->object.oid.hash, abbrev));
} else {
struct strbuf buf = STRBUF_INIT;
pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
- printf("%c %s %s\n", sign,
+ fprintf(file, "%c %s %s\n", sign,
find_unique_abbrev(commit->object.oid.hash, abbrev),
buf.buf);
strbuf_release(&buf);
@@ -1894,7 +1896,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
commit = list->item;
if (has_commit_patch_id(commit, &ids))
sign = '-';
- print_commit(sign, commit, verbose, abbrev);
+ print_commit(sign, commit, verbose, abbrev, revs.diffopt.file);
list = list->next;
}
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 08/10] format-patch: use stdout directly
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
` (6 preceding siblings ...)
2016-06-22 15:01 ` [PATCH v4 07/10] format-patch: avoid freopen() Johannes Schindelin
@ 2016-06-22 15:02 ` Johannes Schindelin
2016-06-24 22:03 ` Junio C Hamano
2016-06-22 15:02 ` [PATCH v4 09/10] shortlog: respect the --output=<file> setting Johannes Schindelin
2016-06-22 15:02 ` [PATCH v4 10/10] Ensure that log respects --output=<file> Johannes Schindelin
9 siblings, 1 reply; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
Earlier, we freopen()ed stdout in order to write patches to files.
That forced us to duplicate stdout (naming it "realstdout") because we
*still* wanted to be able to report the file names.
As we do not abuse stdout that way anymore, we no longer need to
duplicate stdout, either.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/log.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index 869c23b..2a42216 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -805,7 +805,6 @@ static int git_format_config(const char *var, const char *value, void *cb)
return git_log_config(var, value, cb);
}
-static FILE *realstdout = NULL;
static const char *output_directory = NULL;
static int outdir_offset;
@@ -831,7 +830,7 @@ static int open_next_file(struct commit *commit, const char *subject,
fmt_output_subject(&filename, subject, rev);
if (!quiet)
- fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
+ printf("%s\n", filename.buf + outdir_offset);
if ((rev->diffopt.file = fopen(filename.buf, "w")) == NULL)
return error(_("Cannot open patch file %s"), filename.buf);
@@ -1639,9 +1638,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
get_patch_ids(&rev, &ids);
}
- if (!use_stdout)
- realstdout = xfdopen(xdup(1), "w");
-
if (prepare_revision_walk(&rev))
die(_("revision walk setup failed"));
rev.boundary = 1;
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* Re: [PATCH v4 08/10] format-patch: use stdout directly
2016-06-22 15:02 ` [PATCH v4 08/10] format-patch: use stdout directly Johannes Schindelin
@ 2016-06-24 22:03 ` Junio C Hamano
0 siblings, 0 replies; 67+ messages in thread
From: Junio C Hamano @ 2016-06-24 22:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Eric Sunshine
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Earlier, we freopen()ed stdout in order to write patches to files.
> That forced us to duplicate stdout (naming it "realstdout") because we
> *still* wanted to be able to report the file names.
>
> As we do not abuse stdout that way anymore, we no longer need to
> duplicate stdout, either.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/log.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
The logical consequence of 07/10; very nice.
^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v4 09/10] shortlog: respect the --output=<file> setting
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
` (7 preceding siblings ...)
2016-06-22 15:02 ` [PATCH v4 08/10] format-patch: use stdout directly Johannes Schindelin
@ 2016-06-22 15:02 ` Johannes Schindelin
2016-06-22 15:02 ` [PATCH v4 10/10] Ensure that log respects --output=<file> Johannes Schindelin
9 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
Thanks to the diff option parsing, we already know about this option.
We just have to make use of it.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/shortlog.c | 4 +++-
t/t4201-shortlog.sh | 6 ++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 39d74fe..be80547 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -229,7 +229,6 @@ void shortlog_init(struct shortlog *log)
log->wrap = DEFAULT_WRAPLEN;
log->in1 = DEFAULT_INDENT1;
log->in2 = DEFAULT_INDENT2;
- log->file = stdout;
}
int cmd_shortlog(int argc, const char **argv, const char *prefix)
@@ -277,6 +276,7 @@ parse_done:
log.user_format = rev.commit_format == CMIT_FMT_USERFORMAT;
log.abbrev = rev.abbrev;
+ log.file = rev.diffopt.file;
/* assume HEAD if from a tty */
if (!nongit && !rev.pending.nr && isatty(0))
@@ -290,6 +290,8 @@ parse_done:
get_from_rev(&rev, &log);
shortlog_output(&log);
+ if (log.file != stdout)
+ fclose(log.file);
return 0;
}
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index a977365..bd699e1 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -184,4 +184,10 @@ test_expect_success 'shortlog with revision pseudo options' '
git shortlog --exclude=refs/heads/m* --all
'
+test_expect_success 'shortlog with --output=<file>' '
+ git shortlog --output=shortlog master >output &&
+ test ! -s output &&
+ test_line_count = 7 shortlog
+'
+
test_done
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread* [PATCH v4 10/10] Ensure that log respects --output=<file>
2016-06-22 15:01 ` [PATCH v4 00/10] " Johannes Schindelin
` (8 preceding siblings ...)
2016-06-22 15:02 ` [PATCH v4 09/10] shortlog: respect the --output=<file> setting Johannes Schindelin
@ 2016-06-22 15:02 ` Johannes Schindelin
9 siblings, 0 replies; 67+ messages in thread
From: Johannes Schindelin @ 2016-06-22 15:02 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine
The test script t4202-log.sh is already pretty long, and it is a good
idea to test --output with a more obscure option, anyway. So let's
test it in conjunction with line-log.
The most important part of this test, of course, is to ensure that the
file is not closed after writing the diff, but only at the very end
of the log output. That is the entire reason why the test tries to
generate a log that covers more than one commit.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t4211-line-log.sh | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 4451127..9d87777 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -99,4 +99,11 @@ test_expect_success '-L with --first-parent and a merge' '
git log --first-parent -L 1,1:b.c
'
+test_expect_success '-L with --output' '
+ git checkout parallel-change &&
+ git log --output=log -L :main:b.c >output &&
+ test ! -s output &&
+ test_line_count = 70 log
+'
+
test_done
--
2.9.0.118.g0e1a633
^ permalink raw reply related [flat|nested] 67+ messages in thread