Re: [PATCH v3 1/2] add --recode-patch option to git-mailinfo

[Zhang Le <r0bertz@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 3646 bytes --]

On 12:27 Wed 16 Jun     , Junio C Hamano wrote:
> Zhang Le <r0bertz@gentoo.org> writes:
> 
> > diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt
> > index 3ea5aad..24d5bd7 100644
> > --- a/Documentation/git-mailinfo.txt
> > +++ b/Documentation/git-mailinfo.txt
> > @@ -45,7 +45,7 @@ OPTIONS
> >  	them.  This used to be optional but now it is the default.
> >  +
> >  Note that the patch is always used as-is without charset
> > -conversion, even with this flag.
> > +conversion, even with this flag.  Unless --recode-patch is used.
> 
> Somehow this doesn't rhyme well.  Perhaps
> 
>     Note that the patch is used as-with without charset conversion; use
>     `--recode-patch` for that.
> 
> would be better?

OK

> 
> > @@ -54,6 +54,10 @@ conversion, even with this flag.
> >  -n::
> >  	Disable all charset re-coding of the metadata.
> >  
> > +--recode-patch::
> > +	Similar to -u.  But what is re-coded is the patch instead of the
> > +	metainfo.  The default is off.
> 
> Ditto.
> 
> 	Convert the patch from the e-mail to UTF-8 (or the value of the
> 	configuration variable `i18n.commitencoding`, if it is set).
> 

OK

> By the way, what happens when somebody runs the following command?
> 
> 	git mailinfo -n --recode-patch
> 
> Is it desirable?  If not, what _should_ happen instead?

This is not desirable. If -n is used, no recoding will happen.
Maybe we should warn users.
Maybe we use a separate variable like patch_charset, instead of reusing
metainfo_charset?

> 
> > diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
> > index 4a9729b..bb87b32 100644
> > --- a/builtin/mailinfo.c
> > +++ b/builtin/mailinfo.c
> > @@ -12,6 +12,7 @@ static FILE *cmitmsg, *patchfile, *fin, *fout;
> >  static int keep_subject;
> >  static int keep_non_patch_brackets_in_subject;
> >  static const char *metainfo_charset;
> > +static int recode_patch;
> >  static struct strbuf line = STRBUF_INIT;
> >  static struct strbuf name = STRBUF_INIT;
> >  static struct strbuf email = STRBUF_INIT;
> > @@ -828,8 +829,10 @@ static int handle_commit_msg(struct strbuf *line)
> >  	return 0;
> >  }
> >  
> > -static void handle_patch(const struct strbuf *line)
> > +static void handle_patch(struct strbuf *line)
> >  {
> > +	if (recode_patch)
> > +		convert_to_utf8(line, charset.buf);
> >  	fwrite(line->buf, 1, line->len, patchfile);
> >  	patch_lines++;
> >  }
> > @@ -1021,7 +1024,7 @@ static int git_mailinfo_config(const char *var, const char *value, void *unused)
> >  }
> >  
> >  static const char mailinfo_usage[] =
> > -	"git mailinfo [-k|-b] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] msg patch < mail >info";
> > +	"git mailinfo [-k|-b] [-u | --encoding=<encoding> | -n] [--recode-patch] [--scissors | --no-scissors] msg patch < mail >info";
> >  
> >  int cmd_mailinfo(int argc, const char **argv, const char *prefix)
> >  {
> > @@ -1034,6 +1037,7 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
> >  
> >  	def_charset = (git_commit_encoding ? git_commit_encoding : "UTF-8");
> >  	metainfo_charset = def_charset;
> > +	recode_patch = 0;
> 
> Do you need this assignment?

Of course, It is not required. Since variable in bss secton will be initialized
to 0. I just thought maybe it is better to initialized it explicitly, to make
it clear. Of course, this is only personal taste.

I will remove it, if that will suit git's coding style well. :)

-- 
Zhang, Le
Gentoo/Loongson Developer
http://zhangle.is-a-geek.org
0260 C902 B8F8 6506 6586 2B90 BC51 C808 1E4E 2973

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]