[PATCH] git-name-rev: allow --name-only in combination with --stdin

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

[PATCH] git-name-rev: allow --name-only in combination with --stdin

Pieter de Bie-3

Signed-off-by: Pieter de Bie <[hidden email]>
---

        Or was there a specific reason not to allow this?

 Documentation/git-name-rev.txt |    3 +--
 builtin-name-rev.c             |   10 ++++++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 6e77ab1..c8a72dd 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -38,8 +38,7 @@ OPTIONS
  Instead of printing both the SHA-1 and the name, print only
  the name.  If given with --tags the usual tag prefix of
  "tags/" is also omitted from the name, matching the output
- of 'git-describe' more closely.  This option
- cannot be combined with --stdin.
+ of 'git-describe' more closely.
 
 --no-undefined::
  Die with error code != 0 when a reference is undefined,
diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 85612c4..0536af4 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -266,8 +266,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
  if (!name)
  continue;
 
- fwrite(p_start, p - p_start + 1, 1, stdout);
- printf(" (%s)", name);
+ if (data.name_only) {
+ fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
+ printf(name);
+ }
+ else {
+ fwrite(p_start, p - p_start + 1, 1, stdout);
+ printf(" (%s)", name);
+ }
  p_start = p + 1;
  }
  }
--
1.6.0.rc1.163.gc85c5.dirty


--
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] git-name-rev: allow --name-only in combination with --stdin

Junio C Hamano
Pieter de Bie <[hidden email]> writes:

> Signed-off-by: Pieter de Bie <[hidden email]>
> ---
>
> Or was there a specific reason not to allow this?

I'll let Dscho answer that one.

> diff --git a/builtin-name-rev.c b/builtin-name-rev.c
> index 85612c4..0536af4 100644
> --- a/builtin-name-rev.c
> +++ b/builtin-name-rev.c
> @@ -266,8 +266,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
>   if (!name)
>   continue;
>  
> - fwrite(p_start, p - p_start + 1, 1, stdout);
> - printf(" (%s)", name);
> + if (data.name_only) {
> + fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
> + printf(name);
> + }
> + else {
> + fwrite(p_start, p - p_start + 1, 1, stdout);
> + printf(" (%s)", name);
> + }
>   p_start = p + 1;
>   }
>   }

Is it just me to find that this part is getting indented too deeply to be
readable?
--
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] git-name-rev: allow --name-only in combination with --stdin

Johannes Schindelin
Hi,

On Fri, 1 Aug 2008, Junio C Hamano wrote:

> Pieter de Bie <[hidden email]> writes:
>
> > Signed-off-by: Pieter de Bie <[hidden email]>
> > ---
> >
> > Or was there a specific reason not to allow this?
>
> I'll let Dscho answer that one.

... who let's Shawn answer that one.

> > diff --git a/builtin-name-rev.c b/builtin-name-rev.c
> > index 85612c4..0536af4 100644
> > --- a/builtin-name-rev.c
> > +++ b/builtin-name-rev.c
> > @@ -266,8 +266,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> >   if (!name)
> >   continue;
> >  
> > - fwrite(p_start, p - p_start + 1, 1, stdout);
> > - printf(" (%s)", name);
> > + if (data.name_only) {
> > + fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
> > + printf(name);
> > + }
> > + else {
> > + fwrite(p_start, p - p_start + 1, 1, stdout);
> > + printf(" (%s)", name);
> > + }
> >   p_start = p + 1;
> >   }
> >   }
>
> Is it just me to find that this part is getting indented too deeply to be
> readable?

No.

Ciao,
Dscho

--
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] builtin-name-rev: refactor stdin handling to its own function

Pieter de Bie-3

Signed-off-by: Pieter de Bie <[hidden email]>
---

    On 1 aug 2008, at 09:23, Junio C Hamano wrote:
    >Is it just me to find that this part is getting indented too deeply to be
    >readable?

    How about something like this then?

 builtin-name-rev.c |   93 ++++++++++++++++++++++++++++------------------------
 1 files changed, 50 insertions(+), 43 deletions(-)

diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 0536af4..057172d 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -176,6 +176,52 @@ static char const * const name_rev_usage[] = {
  NULL
 };
 
+static void handle_stdin_line(char *p_start, int name_only)
+{
+ int forty = 0;
+ char *p;
+ for (p = p_start; *p; p++) {
+#define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))
+ if (!ishex(*p))
+ forty = 0;
+ else if (++forty == 40 && !ishex(*(p+1))) {
+ unsigned char sha1[40];
+ const char *name = NULL;
+ char c = *(p+1);
+
+ forty = 0;
+
+ *(p+1) = 0;
+ if (!get_sha1(p - 39, sha1)) {
+ struct object *o =
+ lookup_object(sha1);
+ if (o)
+ name = get_rev_name(o);
+ }
+ *(p+1) = c;
+
+ if (!name)
+ continue;
+
+ if (name_only) {
+ fwrite(p_start, p - p_start + 1 - 40,
+ 1, stdout);sssss
+ printf(name);
+ }
+ else {
+ fwrite(p_start, p - p_start + 1, 1, stdout);
+ printf(" (%s)", name);
+ }
+ p_start = p + 1;
+ }
+ }
+
+ /* flush */
+ if (p != p_start)
+ fwrite(p_start, p - p_start, 1, stdout);
+
+}
+
 int cmd_name_rev(int argc, const char **argv, const char *prefix)
 {
  struct object_array revs = { 0, 0, NULL };
@@ -234,53 +280,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
 
  if (transform_stdin) {
  char buffer[2048];
- char *p, *p_start;
+ char *p_start;
 
  while (!feof(stdin)) {
- int forty = 0;
- p = fgets(buffer, sizeof(buffer), stdin);
- if (!p)
+ p_start = fgets(buffer, sizeof(buffer), stdin);
+ if (!p_start)
  break;
 
- for (p_start = p; *p; p++) {
-#define ishex(x) (isdigit((x)) || ((x) >= 'a' && (x) <= 'f'))
- if (!ishex(*p))
- forty = 0;
- else if (++forty == 40 &&
- !ishex(*(p+1))) {
- unsigned char sha1[40];
- const char *name = NULL;
- char c = *(p+1);
-
- forty = 0;
-
- *(p+1) = 0;
- if (!get_sha1(p - 39, sha1)) {
- struct object *o =
- lookup_object(sha1);
- if (o)
- name = get_rev_name(o);
- }
- *(p+1) = c;
-
- if (!name)
- continue;
-
- if (data.name_only) {
- fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
- printf(name);
- }
- else {
- fwrite(p_start, p - p_start + 1, 1, stdout);
- printf(" (%s)", name);
- }
- p_start = p + 1;
- }
- }
-
- /* flush */
- if (p_start != p)
- fwrite(p_start, p - p_start, 1, stdout);
+ handle_stdin_line(p_start, data.name_only);
  }
  } else if (all) {
  int i, max;
--
1.6.0.rc1.214.g5f0bd


--
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] builtin-name-rev: refactor stdin handling to its own function

Junio C Hamano
Pieter de Bie <[hidden email]> writes:

> Signed-off-by: Pieter de Bie <[hidden email]>
> ---
>
>     On 1 aug 2008, at 09:23, Junio C Hamano wrote:
>     >Is it just me to find that this part is getting indented too deeply to be
>     >readable?
>
>     How about something like this then?

Much nicer, except that this refactoring should come first and then a new
feature.  Dropping those extra five 's' so that it would compile would be
a nice bonus as well ;-)

> ...
> +
> + if (name_only) {
> + fwrite(p_start, p - p_start + 1 - 40,
> + 1, stdout);sssss
> + printf(name);
> + }
--
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] git-name-rev: allow --name-only in combination with --stdin

Junio C Hamano
In reply to this post by Pieter de Bie-3
Pieter de Bie <[hidden email]> writes:

> Or was there a specific reason not to allow this?

The --name-only option to the command was added in 1.5.3 cycle, but the
original focus was only to support describe.  I do not see any fundamental
reason to disallow it --- we could even say this is a bug.

I've applied the "split single line processing to a separate function" and
then this patch to 'maint' as a bugfix.

--
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] git-name-rev: don't use printf without format

René Scharfe
printf() without an explicit format string is not a good coding practise,
unless the printed string is guaranteed to not contain percent signs.  While
fixing this, we might as well combine the calls to fwrite() and printf().

Signed-off-by: Rene Scharfe <[hidden email]>
---
 builtin-name-rev.c |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/builtin-name-rev.c b/builtin-name-rev.c
index 7055ac3..08c8aab 100644
--- a/builtin-name-rev.c
+++ b/builtin-name-rev.c
@@ -189,6 +189,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)
  unsigned char sha1[40];
  const char *name = NULL;
  char c = *(p+1);
+ int p_len = p - p_start + 1;
 
  forty = 0;
 
@@ -204,13 +205,10 @@ static void name_rev_line(char *p, struct name_ref_data *data)
  if (!name)
  continue;
 
- if (data->name_only) {
- fwrite(p_start, p - p_start + 1 - 40, 1, stdout);
- printf(name);
- } else {
- fwrite(p_start, p - p_start + 1, 1, stdout);
- printf(" (%s)", name);
- }
+ if (data->name_only)
+ printf("%.*s%s", p_len - 40, p_start, name);
+ else
+ printf("%.*s (%s)", p_len, p_start, name);
  p_start = p + 1;
  }
  }

--
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] git-name-rev: don't use printf without format

Junio C Hamano
René Scharfe <[hidden email]> writes:

> printf() without an explicit format string is not a good coding practise,
> unless the printed string is guaranteed to not contain percent signs.  While
> fixing this, we might as well combine the calls to fwrite() and printf().

Good catch; I should have caught it when I applied the "split overlong
function" patch, but I apparently was blind.

Thanks.


--
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