[PATCH] upload-pack.c: use of parse-options API

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] upload-pack.c: use of parse-options API

Antoine Queru
Option parsing now uses the parser API instead of a local parser.
Code is now more compact.

Signed-off-by: Antoine Queru <[hidden email]>
Signed-off-by: Matthieu Moy <[hidden email]>
---
This is our first project as a warm up. It was taken from the GSoC microproject list.
 upload-pack.c | 51 ++++++++++++++++-----------------------------------
 1 file changed, 16 insertions(+), 35 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index dc802a0..80f65eb 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -14,8 +14,12 @@
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
+#include "parse-options.h"
 
-static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
+static const char * const upload_pack_usage[] = {
+ N_("git upload-pack [--strict] [--timeout=<n>] <dir>"),
+ NULL
+};
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE (1u << 11)
@@ -820,49 +824,26 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 int main(int argc, char **argv)
 {
  char *dir;
- int i;
  int strict = 0;
+ struct option options[] = {
+ OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
+ OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
+ OPT_BOOL(0, "strict", &strict, NULL),
+ OPT_INTEGER(0, "timeout", &timeout, NULL),
+ OPT_END()
+ };
 
  git_setup_gettext();
 
  packet_trace_identity("upload-pack");
  git_extract_argv0_path(argv[0]);
  check_replace_refs = 0;
-
- for (i = 1; i < argc; i++) {
- char *arg = argv[i];
-
- if (arg[0] != '-')
- break;
- if (!strcmp(arg, "--advertise-refs")) {
- advertise_refs = 1;
- continue;
- }
- if (!strcmp(arg, "--stateless-rpc")) {
- stateless_rpc = 1;
- continue;
- }
- if (!strcmp(arg, "--strict")) {
- strict = 1;
- continue;
- }
- if (starts_with(arg, "--timeout=")) {
- timeout = atoi(arg+10);
- daemon_mode = 1;
- continue;
- }
- if (!strcmp(arg, "--")) {
- i++;
- break;
- }
- }
-
- if (i != argc-1)
- usage(upload_pack_usage);
-
+ argc = parse_options(argc, (const char **)argv, NULL, options, upload_pack_usage, 0);
+ if (timeout != 0)
+ daemon_mode = 1;
  setup_path();
 
- dir = argv[i];
+ dir = argv[0];
 
  if (!enter_repo(dir, strict))
  die("'%s' does not appear to be a git repository", dir);
--
2.8.2.403.gaa9c3f6

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] upload-pack.c: use of parse-options API

Jeff King
On Wed, May 18, 2016 at 06:40:19PM +0200, Antoine Queru wrote:

> Option parsing now uses the parser API instead of a local parser.
> Code is now more compact.

Sounds like a good goal.

> -static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
> +static const char * const upload_pack_usage[] = {
> + N_("git upload-pack [--strict] [--timeout=<n>] <dir>"),
> + NULL
> +};

Do we need to enumerate the options here now? The usage message should
list the options from "struct options", which make these redundant.

Something like:

  git -upload-pack [options] <dir>

probably makes more sense.

Of course, it's hard to read the usage message because...

> + struct option options[] = {
> + OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
> + OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
> + OPT_BOOL(0, "strict", &strict, NULL),
> + OPT_INTEGER(0, "timeout", &timeout, NULL),
> + OPT_END()
> + };

You've left the description text for each of these options as NULL, so
running "git-upload-pack -h" segfaults.

I'm not sure whether it is worth hiding the first two options. We
typically hide "internal" options like this for user-facing programs, so
as not to clutter the "-h" output. But upload-pack isn't a user-facing
program. Anybody who is calling it directly with "-h" may be interested
in even its more esoteric options.

> - for (i = 1; i < argc; i++) {
> - char *arg = argv[i];
> -
> - if (arg[0] != '-')
> - break;
> - if (!strcmp(arg, "--advertise-refs")) {
> - advertise_refs = 1;
> - continue;
> - }
> - if (!strcmp(arg, "--stateless-rpc")) {
> - stateless_rpc = 1;
> - continue;
> - }
> - if (!strcmp(arg, "--strict")) {
> - strict = 1;
> - continue;
> - }
> - if (starts_with(arg, "--timeout=")) {
> - timeout = atoi(arg+10);
> - daemon_mode = 1;
> - continue;
> - }
> - if (!strcmp(arg, "--")) {
> - i++;
> - break;
> - }
> - }

A common pitfall in converting a for-loop to parse-options is when there
is anything stateful in the parsing loop. But I don't see anything here.
The trickiest thing is that --timeout implies daemon_mode. You've
handled that below as:

> + if (timeout != 0)
> + daemon_mode = 1;

I think that is OK. It would not be correct if, for example, some other
code set "timeout" to a non-zero value (e.g., a config option), but I
don't see that here.

As a style nit, we usually spell comparison-with-zero as just:

  if (timeout)
        daemon_mode = 1;

Looking at at daemon_mode, though, it appears that it cannot be set in
any other way except here. And it does nothing except to disable
progress in some cases. Weird. There may be some opportunities for
refactoring or simplification there, but I that is outside the scope of
your patch.

> + argc = parse_options(argc, (const char **)argv, NULL, options, upload_pack_usage, 0);

Perhaps this is a good opportunity to use "const" in the declaration of
main(), as most other git programs do. Then you can drop this cast.

>   setup_path();
>  
> - dir = argv[i];
> + dir = argv[0];
>  
>   if (!enter_repo(dir, strict))
>   die("'%s' does not appear to be a git repository", dir);

Prior to your patch, we used to check:

  -       if (i != argc-1)
  -               usage(upload_pack_usage);

which ensured that "dir" was non-NULL. But with your patch, we may pass
NULL to enter_repo. It fortunately catches this, but then we pass the
NULL to die, which can segfault (though on glibc systems, stdio is kind
enough to replace it with the "(null)").

Related, we silently accept extra arguments after your patch (whereas
before we showed the usage message). You probably want to check "argc ==
1", and otherwise show the usage message.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] upload-pack.c: use of parse-options API

Antoine Queru
Thanks for your input.

> > -static const char upload_pack_usage[] = "git upload-pack [--strict]
> > [--timeout=<n>] <dir>";
> > +static const char * const upload_pack_usage[] = {
> > + N_("git upload-pack [--strict] [--timeout=<n>] <dir>"),
> > + NULL
> > +};
>
> Do we need to enumerate the options here now? The usage message should
> list the options from "struct options", which make these redundant.
>
> Something like:
>
>   git -upload-pack [options] <dir>
>
> probably makes more sense.
>

Yes, you are right.

> Of course, it's hard to read the usage message because...
>
> > + struct option options[] = {
> > + OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
> > + OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
> > + OPT_BOOL(0, "strict", &strict, NULL),
> > + OPT_INTEGER(0, "timeout", &timeout, NULL),
> > + OPT_END()
> > + };
>
> You've left the description text for each of these options as NULL, so
> running "git-upload-pack -h" segfaults.
>
> I'm not sure whether it is worth hiding the first two options. We
> typically hide "internal" options like this for user-facing programs, so
> as not to clutter the "-h" output. But upload-pack isn't a user-facing
> program. Anybody who is calling it directly with "-h" may be interested
> in even its more esoteric options.
>

In fact, to do this, I looked at builtin/receive-pack.c, where the parser API
was already implemented, and these first two options were hidden. There were
also no description for any options, so I thought it was not needed. Maybe we
could update this file too ?

> As a style nit, we usually spell comparison-with-zero as just:
>
>   if (timeout)
> daemon_mode = 1;

Because timeout is an int, I personnally think it is more understable to
treat it as it. But I'll update itfor consistency.

> > + argc = parse_options(argc, (const char **)argv, NULL, options,
> > upload_pack_usage, 0);
>
> Perhaps this is a good opportunity to use "const" in the declaration of
> main(), as most other git programs do. Then you can drop this cast.
>

Ok.

> >   setup_path();
> >  
> > - dir = argv[i];
> > + dir = argv[0];
> >  
> >   if (!enter_repo(dir, strict))
> >   die("'%s' does not appear to be a git repository", dir);
>
> Prior to your patch, we used to check:
>
>   -       if (i != argc-1)
>   -               usage(upload_pack_usage);
>
> which ensured that "dir" was non-NULL. But with your patch, we may pass
> NULL to enter_repo. It fortunately catches this, but then we pass the
> NULL to die, which can segfault (though on glibc systems, stdio is kind
> enough to replace it with the "(null)").
>
> Related, we silently accept extra arguments after your patch (whereas
> before we showed the usage message). You probably want to check "argc ==
> 1", and otherwise show the usage message.

Ok.

-Antoine
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] upload-pack.c: use of parse-options API

Jeff King
On Thu, May 19, 2016 at 12:10:31PM +0200, Antoine Queru wrote:

> > I'm not sure whether it is worth hiding the first two options. We
> > typically hide "internal" options like this for user-facing programs, so
> > as not to clutter the "-h" output. But upload-pack isn't a user-facing
> > program. Anybody who is calling it directly with "-h" may be interested
> > in even its more esoteric options.
>
> In fact, to do this, I looked at builtin/receive-pack.c, where the parser API
> was already implemented, and these first two options were hidden. There were
> also no description for any options, so I thought it was not needed. Maybe we
> could update this file too ?

Yeah, I don't think it's that bad to hide them, and perhaps consistency
with receive-pack is better. AFAICT, descriptions for hidden options are
pointless; they are never shown (in fact, it seems like OPT_HIDDEN_BOOL
probably shouldn't even take such an option?).

But the non-hidden options _do_ need non-NULL descriptions.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] upload-pack.c: use of parse-options API

Matthieu Moy-2
Jeff King wrote:

> On Thu, May 19, 2016 at 12:10:31PM +0200, Antoine Queru wrote:
>
> > > I'm not sure whether it is worth hiding the first two options. We
> > > typically hide "internal" options like this for user-facing programs, so
> > > as not to clutter the "-h" output. But upload-pack isn't a user-facing
> > > program. Anybody who is calling it directly with "-h" may be interested
> > > in even its more esoteric options.
> >
> > In fact, to do this, I looked at builtin/receive-pack.c, where the parser
> > API
> > was already implemented, and these first two options were hidden. There
> > were
> > also no description for any options, so I thought it was not needed. Maybe
> > we
> > could update this file too ?
>
> Yeah, I don't think it's that bad to hide them, and perhaps consistency
> with receive-pack is better.

IIRC, part of the reason receive-pack has hidden options is that it was a
GSoC microproject, and writing an accurate description is much harder than
what we expect from a microproject.

IOW, I'm all for un-hiding these options, but that shouldn't be a requirement
for a beginner's project.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] upload-pack.c: use of parse-options API

Antoine Queru
In reply to this post by Antoine Queru
Option parsing now uses the parser API instead of a local parser.
Code is now more compact.
Description for -stateless-rpc and --advertise-refs
come from the commit (gmane/131517) where there were implemented.

Signed-off-by: Antoine Queru <[hidden email]>
Signed-off-by: Matthieu Moy <[hidden email]>
---

diff v1 v2:
Usage display "[options]" instead of "[--strict] [--timeout=<n>]".
"argv" is now "const char **".
"-stateless-rpc and --advertise-refs" are no more hidden.
Description has been added for every option.
Usage is displayed if there is not exactly one non option argument.

If we agree on not having hidden option anymore, I will update the doc.
 upload-pack.c | 62 +++++++++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 36 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index dc802a0..a8617ac 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -14,8 +14,12 @@
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
+#include "parse-options.h"
 
-static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
+static const char * const upload_pack_usage[] = {
+ N_("git upload-pack [options] <dir>"),
+ NULL
+};
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE (1u << 11)
@@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
  return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-int main(int argc, char **argv)
+int main(int argc, const char **argv)
 {
- char *dir;
- int i;
+ const char *dir;
  int strict = 0;
+ struct option options[] = {
+ OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
+ N_("may perform only a single read-write cycle with stdin and stdout")),
+ OPT_BOOL(0, "advertise-refs", &advertise_refs,
+ N_("only the initial ref advertisement is output, program exits immediately")),
+ OPT_BOOL(0, "strict", &strict,
+ N_("do not try <directory>/.git/ if <directory> is no Git directory")),
+ OPT_INTEGER(0, "timeout", &timeout,
+    N_("interrupt transfer after <n> seconds of inactivity")),
+ OPT_END()
+ };
 
  git_setup_gettext();
 
@@ -829,41 +843,17 @@ int main(int argc, char **argv)
  git_extract_argv0_path(argv[0]);
  check_replace_refs = 0;
 
- for (i = 1; i < argc; i++) {
- char *arg = argv[i];
-
- if (arg[0] != '-')
- break;
- if (!strcmp(arg, "--advertise-refs")) {
- advertise_refs = 1;
- continue;
- }
- if (!strcmp(arg, "--stateless-rpc")) {
- stateless_rpc = 1;
- continue;
- }
- if (!strcmp(arg, "--strict")) {
- strict = 1;
- continue;
- }
- if (starts_with(arg, "--timeout=")) {
- timeout = atoi(arg+10);
- daemon_mode = 1;
- continue;
- }
- if (!strcmp(arg, "--")) {
- i++;
- break;
- }
- }
-
- if (i != argc-1)
- usage(upload_pack_usage);
+ argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
+
+ if (argc != 1)
+ usage_with_options(upload_pack_usage, options);
 
- setup_path();
+ if (timeout)
+ daemon_mode = 1;
 
- dir = argv[i];
+ setup_path();
 
+ dir = argv[0];
  if (!enter_repo(dir, strict))
  die("'%s' does not appear to be a git repository", dir);
 
--
2.8.2.403.gf2352ca

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] upload-pack.c: use of parse-options API

Matthieu Moy-2
[hidden email] wrote:
> Option parsing now uses the parser API instead of a local parser.
> Code is now more compact.
> Description for -stateless-rpc and --advertise-refs
> come from the commit (gmane/131517)

Please, use a real commit id instead of a Gmane link.

We don't know how long Gmane will remain up, but a self
reference from Git's history to itself will always remain valid.

The following alias is handy for this:

[alias]
        whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short

In your case it would allow writing:

Description for --stateless-rpc and --advertise-refs is taken
from commit 42526b4 (Add stateless RPC options to upload-pack,
receive-pack, 2009-10-30).

> diff v1 v2:

We usually say "diff" to refer to an actual diff. I'd write "changes since v1" here.

> + OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
> + N_("may perform only a single read-write cycle with stdin and stdout")),

It's weird to define what an option does with "may". It's a
property of --stateless-rpc, but does not really define it.

> + if (argc != 1)
> + usage_with_options(upload_pack_usage, options);
>  
> - setup_path();
> + if (timeout)
> + daemon_mode = 1;
>  
> - dir = argv[i];
> + setup_path();
>  
> + dir = argv[0];

Not a problem with your code, but the patch shows "setup_path()"
as moved while it is not really. Maybe using "send-email
--patience" or some other diff option could make the patch nicer.
Not really important as it does not change the final state.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] upload-pack.c: use of parse-options API

Junio C Hamano
Matthieu Moy <[hidden email]> writes:

> [hidden email] wrote:
>> Option parsing now uses the parser API instead of a local parser.
>> Code is now more compact.
>> Description for -stateless-rpc and --advertise-refs
>> come from the commit (gmane/131517)
>
> Please, use a real commit id instead of a Gmane link.
>
> We don't know how long Gmane will remain up, but a self
> reference from Git's history to itself will always remain valid.
>
> The following alias is handy for this:
>
> [alias]
>         whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short
>
> In your case it would allow writing:
>
> Description for --stateless-rpc and --advertise-refs is taken
> from commit 42526b4 (Add stateless RPC options to upload-pack,
> receive-pack, 2009-10-30).

Good suggestion; a real question may be how you went from
$gmane/131517 to 42526b4 (I vaguely recall that you have and publish
a database of sort; this would be a good place to advertise it ;-).

>
>> diff v1 v2:
>
> We usually say "diff" to refer to an actual diff. I'd write "changes since v1" here.
>
>> + OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
>> + N_("may perform only a single read-write cycle with stdin and stdout")),
>
> It's weird to define what an option does with "may". It's a
> property of --stateless-rpc, but does not really define it.

If this "may" were to express "The program might or might not do
this, and we do not know what it would do until we try", then it
indeed would be weird.  But the way the word "may" was used in
42526b4 is "the program is ALLOWED to do only a single exchange".

I agree that the phrasing is bad, in the sense that it can be
misread as a non-definition; perhaps

    quit after a single request/response exchange

or something?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] upload-pack.c: use of parse-options API

Junio C Hamano
In reply to this post by Matthieu Moy-2
Matthieu Moy <[hidden email]> writes:

>> + if (argc != 1)
>> + usage_with_options(upload_pack_usage, options);
>>  
>> - setup_path();
>> + if (timeout)
>> + daemon_mode = 1;
>>  
>> - dir = argv[i];
>> + setup_path();
>>  
>> + dir = argv[0];
>
> Not a problem with your code, but the patch shows "setup_path()"
> as moved while it is not really. Maybe using "send-email
> --patience" or some other diff option could make the patch nicer.

Encouraging use of "send-email" with "--patience" is a losing
approach if your goal is to present more reviewable diff, isn't it?
Using "send-email" as if it is a front-end of "format-patch" means
you lose the opportunity for the final proof-reading (not just
finding typoes in the message, but the shape of diff, like you are
pointing out).

Using "format-patch --patience" or some other diff option, and pick
the best one to give to "send-email" would indeed be a way to do so.

> Not really important as it does not change the final state.

I wondered if this is an example of real-world fallout from
0018da1^2~1 (xdiff: implement empty line chunk heuristic,
2016-04-19), but it does not seem to be so.

What is happening is that Antoine's patch (which is slightly
different from what you quoted above) has trailing whitespace after
"setup_path();", so it indeed is the original setup_path(); is
removed, a few lines were inserted, argv[i] reference is removed
and then a totally different "setup_path(); " was added there.

With that whitespace-breakage corrected, the resulting patch ends
more like this:

+ if (argc != 1)
+ usage_with_options(upload_pack_usage, options);
 
- if (i != argc-1)
- usage(upload_pack_usage);
+ if (timeout)
+ daemon_mode = 1;
 
  setup_path();
 
- dir = argv[i];
-
+ dir = argv[0];
  if (!enter_repo(dir, strict))
  die("'%s' does not appear to be a git repository", dir);
 
which is more reasonable.

So in the end, this was not "Not a problem with your code" ;-) It
was.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] upload-pack.c: use of parse-options API

Matthieu Moy-2
Junio C Hamano wrote:
> Encouraging use of "send-email" with "--patience" is a losing
> approach if your goal is to present more reviewable diff, isn't it?
> Using "send-email" as if it is a front-end of "format-patch" means
> you lose the opportunity for the final proof-reading (not just
> finding typoes in the message, but the shape of diff, like you are
> pointing out).
>
> Using "format-patch --patience" or some other diff option, and pick
> the best one to give to "send-email" would indeed be a way to do so.

It's a matter of taste. My flow is "send-email-only", I do as much as
possible in-tree, and when I notice something to do during "git send-email",
I just abort and retry. So "Oops, looks ugly, I'll try with another
option" is OK in this flow.

> What is happening is that Antoine's patch (which is slightly
> different from what you quoted above) has trailing whitespace after
> "setup_path();"

Nice catch!

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] upload-pack.c: use of parse-options API

Matthieu Moy-2
In reply to this post by Junio C Hamano
Junio Hamano wrote:

> Matthieu Moy <[hidden email]> writes:
>
> > [hidden email] wrote:
> >> come from the commit (gmane/131517)
> >
> > Please, use a real commit id instead of a Gmane link.
> >
> > We don't know how long Gmane will remain up, but a self
> > reference from Git's history to itself will always remain valid.
> >
> > The following alias is handy for this:
> >
> > [alias]
> >         whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short
> >
> > In your case it would allow writing:
> >
> > Description for --stateless-rpc and --advertise-refs is taken
> > from commit 42526b4 (Add stateless RPC options to upload-pack,
> > receive-pack, 2009-10-30).
>
> Good suggestion; a real question may be how you went from
> $gmane/131517 to 42526b4 (I vaguely recall that you have and publish
> a database of sort; this would be a good place to advertise it ;-).

I don't (I think someone has, but I'm not this someone).

I just "git log --grep"-ed the subject line in Git's history.

>     quit after a single request/response exchange

Seems much clearer to me.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] upload-pack.c: use of parse-options API

Junio C Hamano
In reply to this post by Matthieu Moy-2
Matthieu Moy <[hidden email]> writes:

>> Using "format-patch --patience" or some other diff option, and pick
>> the best one to give to "send-email" would indeed be a way to do so.
>
> It's a matter of taste. My flow is "send-email-only", I do as much as
> possible in-tree, and when I notice something to do during "git send-email",
> I just abort and retry. So "Oops, looks ugly, I'll try with another
> option" is OK in this flow.

Ah, I didn't consider "final-review in send-email and aborting".

I agree that it is just the matter of preference, if it is easy to
review everything that you would be sending out, and decide to
abort.  I just thought that final review would be cumbersome in that
flow.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] upload-pack.c: use of parse-options API

Antoine Queru
In reply to this post by Antoine Queru
Option parsing now uses the parser API instead of a local parser.
Code is now more compact.
Description for -stateless-rpc and --advertise-refs
come from the commit 42526b4 (Add stateless RPC options to upload-pack, receive-pack, 2009-10-30).

Signed-off-by: Antoine Queru <[hidden email]>
Signed-off-by: Matthieu Moy <[hidden email]>
---
Change since v2:

Commit message updated.
WhiteSpace removed after the "setup_path();".
Description for --stateless-rpc changed to : "quit after a single request/response exchange".

For the description, i don't really understand what this option is doing, so i just copy pasted
Junio's sentence.
 upload-pack.c | 59 +++++++++++++++++++++++++----------------------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index dc802a0..2d53c7b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -14,8 +14,12 @@
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
+#include "parse-options.h"
 
-static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
+static const char * const upload_pack_usage[] = {
+ N_("git upload-pack [options] <dir>"),
+ NULL
+};
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE (1u << 11)
@@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
  return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-int main(int argc, char **argv)
+int main(int argc, const char **argv)
 {
- char *dir;
- int i;
+ const char *dir;
  int strict = 0;
+ struct option options[] = {
+ OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
+ N_("quit after a single request/response exchange")),
+ OPT_BOOL(0, "advertise-refs", &advertise_refs,
+ N_("only the initial ref advertisement is output, program exits immediately")),
+ OPT_BOOL(0, "strict", &strict,
+ N_("do not try <directory>/.git/ if <directory> is no Git directory")),
+ OPT_INTEGER(0, "timeout", &timeout,
+    N_("interrupt transfer after <n> seconds of inactivity")),
+ OPT_END()
+ };
 
  git_setup_gettext();
 
@@ -829,40 +843,17 @@ int main(int argc, char **argv)
  git_extract_argv0_path(argv[0]);
  check_replace_refs = 0;
 
- for (i = 1; i < argc; i++) {
- char *arg = argv[i];
-
- if (arg[0] != '-')
- break;
- if (!strcmp(arg, "--advertise-refs")) {
- advertise_refs = 1;
- continue;
- }
- if (!strcmp(arg, "--stateless-rpc")) {
- stateless_rpc = 1;
- continue;
- }
- if (!strcmp(arg, "--strict")) {
- strict = 1;
- continue;
- }
- if (starts_with(arg, "--timeout=")) {
- timeout = atoi(arg+10);
- daemon_mode = 1;
- continue;
- }
- if (!strcmp(arg, "--")) {
- i++;
- break;
- }
- }
+ argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
+
+ if (argc != 1)
+ usage_with_options(upload_pack_usage, options);
 
- if (i != argc-1)
- usage(upload_pack_usage);
+ if (timeout)
+ daemon_mode = 1;
 
  setup_path();
 
- dir = argv[i];
+ dir = argv[0];
 
  if (!enter_repo(dir, strict))
  die("'%s' does not appear to be a git repository", dir);
--
2.8.2.403.gf2352ca

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3] upload-pack.c: use of parse-options API

Matthieu Moy-2
Antoine Queru <[hidden email]> writes:

> Option parsing now uses the parser API instead of a local parser.
> Code is now more compact.
> Description for -stateless-rpc and --advertise-refs
> come from the commit 42526b4 (Add stateless RPC options to upload-pack, receive-pack, 2009-10-30).

We usually wrap commit messages before 80 columns, and separate
paragraphs with blank lines. Have a look at "git log --no-merges" and
compare with your message.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html