threads / patch / 34461

patchTIG: Fix to reinstate proper operation with no arguments

Subject: [PATCH] TIG: Fix to reinstate proper operation with no arguments

## tl;dr

7 messages between Jul 18, 2013 and Jul 29, 2013. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Drew Northup· Jul 18, 2013, 04:51 UTC · lore

Since c7d67ab running "tig" with no options has failed with the error "tig: No revisions match the given arguments." This was due to a change in how the arguments for the back-end git call was being constructed. This change caused the blank field left in place of "(encoding_arg)" when it is empty to not overwrite "buf" which then caused the value in "buf" to be copied into dst_argv twice. The resulting git command failed if there was no available revision named "log" as shown in the trace.

>From the TIG_TRACE log:

git log log --no-color --pretty=raw --parents --parents -- fatal: bad revision 'log'

This fix works by teaching tig that when it is supplied with a blank field in the source argument buffer that it should skip over that field and continue instead of copying the previous field value into the destination buffer a second time.

github issue # 167
Signed-off-by: Drew Northup <n1xim.email@gmail.com>
---

This should apply cleanly to the tig public master whether the mkstemps() patch I wrote has been applied or not.

 tig.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
Show changes to tig.c +7 −2
diff --git a/tig.c b/tig.c
index ba9ba98..1016cfe 100644
--- a/tig.c
+++ b/tig.c
@@ -3105,10 +3105,11 @@ static bool
 format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
 {
 	format->bufpos = 0;
+	int len = 0;
 
 	while (arg) {
 		char *next = strstr(arg, "%(");
-		int len = next ? next - arg : strlen(arg);
+		len = next ? next - arg : strlen(arg);
 
 		if (len && !string_format_from(format->buf, &format->bufpos, "%.*s", len, arg))
 			return FALSE;
@@ -3119,7 +3120,11 @@ format_append_arg(struct format_context *format, const char ***dst_argv, const c
 		arg = next ? strchr(next, ')') + 1 : NULL;
 	}
 
-	return argv_append(dst_argv, format->buf);
+	if(len){
+		return argv_append(dst_argv, format->buf);
+	} else {
+		return TRUE;
+	}
 }
 
 static bool

-- 
1.8.0
Drew Northup· Jul 18, 2013, 13:30 UTC · re: Drew Northup · lore

Re: [PATCH] TIG: Fix to reinstate proper operation with no arguments

Somehow this patch breaks the main view to not open the correct commit in diff view when <enter> is pressed. Back to the debugger...

On 07/18/2013 12:51 AM, Drew Northup wrote:
Show 59 quoted lines
> Since c7d67ab running "tig" with no options has failed with the
> error "tig: No revisions match the given arguments." This was due
> to a change in how the arguments for the back-end git call was
> being constructed. This change caused the blank field left in
> place of "(encoding_arg)" when it is empty to not overwrite
> "buf" which then caused the value in "buf" to be copied into
> dst_argv twice. The resulting git command failed if there was no
> available revision named "log" as shown in the trace.
>
>  From the TIG_TRACE log:
> git log log --no-color --pretty=raw --parents --parents --
> fatal: bad revision 'log'
>
> This fix works by teaching tig that when it is supplied with a
> blank field in the source argument buffer that it should skip
> over that field and continue instead of copying the previous
> field value into the destination buffer a second time.
>
> github issue # 167
>
> Signed-off-by: Drew Northup<n1xim.email@gmail.com>
> ---
>
> This should apply cleanly to the tig public master whether the
> mkstemps() patch I wrote has been applied or not.
>
>   tig.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/tig.c b/tig.c
> index ba9ba98..1016cfe 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3105,10 +3105,11 @@ static bool
>   format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
>   {
>   	format->bufpos = 0;
> +	int len = 0;
>
>   	while (arg) {
>   		char *next = strstr(arg, "%(");
> -		int len = next ? next - arg : strlen(arg);
> +		len = next ? next - arg : strlen(arg);
>
>   		if (len&&  !string_format_from(format->buf,&format->bufpos, "%.*s", len, arg))
>   			return FALSE;
> @@ -3119,7 +3120,11 @@ format_append_arg(struct format_context *format, const char ***dst_argv, const c
>   		arg = next ? strchr(next, ')') + 1 : NULL;
>   	}
>
> -	return argv_append(dst_argv, format->buf);
> +	if(len){
> +		return argv_append(dst_argv, format->buf);
> +	} else {
> +		return TRUE;
> +	}
>   }
>
>   static bool
-- 
--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
Jonas Fonseca· Jul 19, 2013, 04:07 UTC · re: Drew Northup · lore

Re: [PATCH] TIG: Fix to reinstate proper operation with no arguments

On Thu, Jul 18, 2013 at 9:30 AM, Drew Northup <n1xim.email@gmail.com> wrote:
>
> Somehow this patch breaks the main view to not open the correct commit in diff view when <enter> is pressed. Back to the debugger...
Does this (possibly white-space damaged) patch work for you?
Show changes to tig.c +1 −2
diff --git a/tig.c b/tig.c
index ba9ba98..74a2928 100644
--- a/tig.c
+++ b/tig.c
@@ -3104,7 +3104,7 @@ format_expand_arg(struct format_context *format,
const char *name)
 static bool
 format_append_arg(struct format_context *format, const char
***dst_argv, const char *arg)
 {
- format->bufpos = 0;
+ format->buf[0] = format->bufpos = 0;

  while (arg) {
  char *next = strstr(arg, "%(");


--
Jonas Fonseca
Drew Northup· Jul 19, 2013, 11:55 UTC · re: Jonas Fonseca · lore

Re: [PATCH] TIG: Fix to reinstate proper operation with no arguments

On Fri, Jul 19, 2013 at 12:07 AM, Jonas Fonseca <fonseca@diku.dk> wrote:
Show 5 quoted lines
> On Thu, Jul 18, 2013 at 9:30 AM, Drew Northup <n1xim.email@gmail.com> wrote:
>>
>> Somehow this patch breaks the main view to not open the correct commit in diff view when <enter> is pressed. Back to the debugger...
>
> Does this (possibly white-space damaged) patch work for you?

I did look back at that sort of fix, but I kept getting compiler warnings about a pointer conversion. Yes, it does work (I was actually initializing the whole buffer to NULL), but I'd prefer doing so without the implicit int* conversion warnings.

Show 15 quoted lines
> diff --git a/tig.c b/tig.c
> index ba9ba98..74a2928 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3104,7 +3104,7 @@ format_expand_arg(struct format_context *format,
> const char *name)
>  static bool
>  format_append_arg(struct format_context *format, const char
> ***dst_argv, const char *arg)
>  {
> - format->bufpos = 0;
> + format->buf[0] = format->bufpos = 0;
>
>   while (arg) {
>   char *next = strstr(arg, "%(");

-- -Drew Northup -------------------------------------------------------------- "As opposed to vegetable or mineral error?" -John Pescatore, SANS NewsBites Vol. 12 Num. 59

Drew Northup· Jul 24, 2013, 12:29 UTC · re: Jonas Fonseca · lore

Re: [PATCH] TIG: Fix to reinstate proper operation with no arguments

On 07/19/2013 12:07 AM, Jonas Fonseca wrote:
Show 5 quoted lines
> On Thu, Jul 18, 2013 at 9:30 AM, Drew Northup<n1xim.email@gmail.com>  wrote:
>>
>> Somehow this patch breaks the main view to not open the correct commit in diff view when<enter>  is pressed. Back to the debugger...
>
> Does this (possibly white-space damaged) patch work for you?

It does, but I prefer to actually fix the problem this highlights and not to paper it over. Patch V2 will be on the way shortly.

Show 19 quoted lines
> diff --git a/tig.c b/tig.c
> index ba9ba98..74a2928 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3104,7 +3104,7 @@ format_expand_arg(struct format_context *format,
> const char *name)
>   static bool
>   format_append_arg(struct format_context *format, const char
> ***dst_argv, const char *arg)
>   {
> - format->bufpos = 0;
> + format->buf[0] = format->bufpos = 0;
>
>    while (arg) {
>    char *next = strstr(arg, "%(");
>
>
> --
> Jonas Fonseca
-- 
--
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
Drew Northup· Jul 24, 2013, 12:50 UTC · re: Drew Northup · lore

[PATCH V2] TIG: Fix to reinstate proper operation with no arguments

Since c7d67ab running "tig" with no options has failed with the error "tig: No revisions match the given arguments." This was due to a change in how the arguments for the back-end git call was being constructed. This change caused the blank field left in place of "(encoding_arg)" when it is empty to not overwrite "buf" which then caused the value in "buf" to be copied into dst_argv twice. The resulting git command failed if there was no available revision named "log" as shown in the trace.

>From the TIG_TRACE log:

git log log --no-color --pretty=raw --parents --parents -- fatal: bad revision 'log'

This fix works by properly and fully initializing "buf" before each use.
github issue # 167
Helped-by: Jonas Fonseca <fonseca@diku.dk>
Signed-off-by: Drew Northup <n1xim.email@gmail.com>
---

This should apply cleanly to the tig public master whether the mkstemps() patch I wrote has been applied or not. (As of the time I am sending this for review I haven't yet applied the latest fix to my mkstemps() patch that has been submitted for git itself.)

This time, knowing for sure now that format->buf is not being used in the extant code path for any other purpose, I went ahead and initialized the whole thing to be sure that we don't find any other ghosts hiding in that buffer between uses. Just initializing the first byte fixes the near term problem but does not prevent the buffer initialization issue that this bug highlighted from rising up again later on.

 tig.c | 4 ++++
 1 file changed, 4 insertions(+)
Show changes to tig.c +4 −0
diff --git a/tig.c b/tig.c
index ba9ba98..c65bc43 100644
--- a/tig.c
+++ b/tig.c
@@ -3104,8 +3104,12 @@ format_expand_arg(struct format_context *format, const char *name)
 static bool
 format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
 {
+	int i;
 	format->bufpos = 0;
 
+	for (i = 0; i < SIZEOF_STR; i++)
+		format->buf[i] = 0;
+
 	while (arg) {
 		char *next = strstr(arg, "%(");
 		int len = next ? next - arg : strlen(arg);

-- 
1.8.0
Jonas Fonseca· Jul 29, 2013, 01:23 UTC · re: Drew Northup · lore

Re: [PATCH V2] TIG: Fix to reinstate proper operation with no arguments

On Wed, Jul 24, 2013 at 8:50 AM, Drew Northup <n1xim.email@gmail.com> wrote:
Show 7 quoted lines
> This time, knowing for sure now that format->buf is not being used in
> the extant code path for any other purpose, I went ahead and
> initialized the whole thing to be sure that we don't find any other
> ghosts hiding in that buffer between uses. Just initializing the
> first byte fixes the near term problem but does not prevent the buffer
> initialization issue that this bug highlighted from rising up again
> later on.
Thanks applied with minor tidyup.
Show 9 quoted lines
> diff --git a/tig.c b/tig.c
> index ba9ba98..c65bc43 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -3104,8 +3104,12 @@ format_expand_arg(struct format_context *format, const char *name)
>  static bool
>  format_append_arg(struct format_context *format, const char ***dst_argv, const char *arg)
>  {
> +       int i;
Added space after the declaration.
>         format->bufpos = 0;
>
> +       for (i = 0; i < SIZEOF_STR; i++)
Changed this to use sizeof(format->buf) instead.
Show 8 quoted lines
> +               format->buf[i] = 0;
> +
>         while (arg) {
>                 char *next = strstr(arg, "%(");
>                 int len = next ? next - arg : strlen(arg);
> --
> 1.8.0
>
-- 
Jonas Fonseca

← back to recent threads