<carlos.duclos@xxxxxxxxx> writes:
First, the comments below are not indication that this patch is not a
good idea, because in my opinion it is. This review is about
_technical_ shortcomings...
> Short description:
> ==================
> To enable that I added a new option "--output" that will redirect
> the output to a file instead to stdout.
>
> Long description:
> =================
>
> When archiving a repository there is no way to send the result to a
> file without using redirection at shell level. Since there are
> situations where redirection is not available, for instance when
> running git inside a continuous integration system which is already
> redirecting the output, I added an option to write the archive to a
> file directly.
>
> In order to do that I added a new option to archiver_args, int
> output_fd, which holds the file descriptor where the resulting
> archive should be written. If no option is specified in command line
> that option defaults to 1 and no behavior change, however if the
> "--output" option is specified and the file was created that file
> descriptor points to the new file. I also modified the write_or_die
> calls to use "output_fd" instead of 1, so they write to the file
> descriptor.
>
There should be separator, e.g.
-- >8 --
to mark where comments ends and commit message begins.
> From 10e09bf828c18f2846651262b7f647b630e09872 Mon Sep 17 00:00:00 2001
The above line is not necessary.
> From: Carlos Manuel Duclos Vergara <carlos.duclos@xxxxxxxxxxxxx>
> Date: Fri, 13 Feb 2009 16:09:39 +0100
Usually 'From:' is required only if the value from email cannot be
used, either because you send email on behalf of somebody else
(somebody else is author of patch), or because you are sending from
alternate email address.
The 'Date:' header is very rarely required: the one from email should
be good enough in most cases.
> Subject: [PATCH] When archiving a repository there is no way to specify a file as output.
> To enable that I added a new option "--output" that will redirect the output to a file instead to stdout.
Git commit message conventions (see Documentation/SubmittingPatches)
call for short description or a commit/patch (commit summary) in first
line: that is what it would be in email subject ('Subject:' line
above), followed by a more detailed description.
Your summary (subject of patch) is both too long, and not standalone.
I think the better choice would be:
Subject: [PATCH] git-archive: Add new option "--output" to write to a file
When archiving a repository there is no way to specify a file as
output. This patch a new option "--output" that will redirect the
output to a file instead to stdout.
I think however that some of 'long description' above, or perhaps even
whole of it, should be put in commit message itself, and not be only
as a comment (present only in mailing list archives).
>
> Signed-off-by: Carlos Manuel Duclos Vergara <carlos.duclos@xxxxxxxxx>
Why do you use different email address for signoff, and for authorship
('From:' line)? Was it intended, or was it an accident?
[...]
> From 7cbd0a3edb1cf75b5a0644263e1755fd18a5c37d Mon Sep 17 00:00:00 2001
> From: Carlos Manuel Duclos Vergara <carlos.duclos@xxxxxxxxxxxxx>
> Date: Fri, 13 Feb 2009 16:22:21 +0100
> Subject: [PATCH] Updating documentation for git-archive in order to reflect the new "--output" option.
>
> Signed-off-by: Carlos Manuel Duclos Vergara <carlos.duclos@xxxxxxxxx>
First, this should really be together in one, single patch.
Second, there is convention of sending patch series as a _series_ of
emails, eother as responses to cover letter message, or 'chained' so
subsequent patches (emails) are replies to earlier ones. For better
readibility, and in case some patches get lost or be sorted out of
order by email/news client, there is convention of using [PATCH n/m]
prefix.
--
Jakub Narebski
Poland
ShadeHawk on #git
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
