threads / patch / 19754

patchshow-branch: fix segfault when showbranch.default exists

Subject: [PATCH] show-branch: fix segfault when showbranch.default exists

## tl;dr

12 messages between Jun 9, 2009 and Jun 10, 2009. Diffs are folded; open one to read it.

replies: 11people: 9as markdown or json

Junio C Hamano· Jun 9, 2009, 06:26 UTC · lore

When running "git show-branch" without any parameter in a repository that has showbranch.default defined, we used to rely on the fact that our handcrafted option parsing loop never looked at av[0].

The array of default strings had the first real command line argument in default_arg[0], but the option parser wanted to look at the array starting at av[1], so we assigned the address of -1th element to av to force the loop start working from default_arg[0].

This no longer worked since 5734365 (show-branch: migrate to parse-options API, 2009-05-21), as parse_options_start() saved the incoming &av[0] in its ctx->out and later in parse_options_end() it did memmove to ctx->out (with ctx->cpidx == 0), overwriting the memory before default_arg[] array.

I am not sure if this is a bug in parse_options(), or a bug in the caller, and tonight I do not have enough concentration to figure out which. In any case, this patch works the issue around.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-show-branch.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)
Show changes to builtin-show-branch.c +11 −3
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 01bea3b..baec9ed 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -565,7 +565,15 @@ static int git_show_branch_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "showbranch.default")) {
 		if (!value)
 			return config_error_nonbool(var);
-		if (default_alloc <= default_num + 1) {
+		/*
+		 * default_arg is now passed to parse_options(), so we need to
+		 * mimick the real argv a bit better.
+		 */
+		if (!default_num) {
+			default_alloc = 20;
+			default_arg = xcalloc(default_alloc, sizeof(*default_arg));
+			default_arg[default_num++] = "show-branch";
+		} else if (default_alloc <= default_num + 1) {
 			default_alloc = default_alloc * 3 / 2 + 20;
 			default_arg = xrealloc(default_arg, sizeof *default_arg * default_alloc);
 		}
@@ -692,8 +700,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 	/* If nothing is specified, try the default first */
 	if (ac == 1 && default_num) {
-		ac = default_num + 1;
-		av = default_arg - 1; /* ick; we would not address av[0] */
+		ac = default_num;
+		av = default_arg;
 	}
 
 	ac = parse_options(ac, av, prefix, builtin_show_branch_options,
Stephen Boyd· Jun 9, 2009, 07:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] show-branch: fix segfault when showbranch.default exists

Junio C Hamano wrote:
> I am not sure if this is a bug in parse_options(), or a bug in the caller,
> and tonight I do not have enough concentration to figure out which.  In
> any case, this patch works the issue around.

I am low on concentration tonight as well, but this looks right to me. Parse options is expecting the regular old argv and argc. I overlooked this code path during the conversion (though I remember figuring out what this path was doing). Faking the argv and argc a little more accurately, like you do, should work fine.

On a side note, I can't remember why I used PARSE_OPT_STOP_AT_NON_OPTION. I think it should be 0.

Pierre Habouzit· Jun 9, 2009, 08:06 UTC · re: Stephen Boyd · lore

Re: [PATCH] show-branch: fix segfault when showbranch.default exists

On Tue, Jun 09, 2009 at 12:17:28AM -0700, Stephen Boyd wrote:
Show 10 quoted lines
> Junio C Hamano wrote:
> > I am not sure if this is a bug in parse_options(), or a bug in the caller,
> > and tonight I do not have enough concentration to figure out which.  In
> > any case, this patch works the issue around.
> 
> I am low on concentration tonight as well, but this looks right to me.
> Parse options is expecting the regular old argv and argc. I overlooked
> this code path during the conversion (though I remember figuring out
> what this path was doing). Faking the argv and argc a little more
> accurately, like you do, should work fine.
yes, that's it.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Junio C Hamano· Jun 9, 2009, 16:28 UTC · re: Pierre Habouzit · lore

Re: [PATCH] show-branch: fix segfault when showbranch.default exists

Pierre Habouzit <madcoder@madism.org> writes:
Show 13 quoted lines
> On Tue, Jun 09, 2009 at 12:17:28AM -0700, Stephen Boyd wrote:
>> Junio C Hamano wrote:
>> > I am not sure if this is a bug in parse_options(), or a bug in the caller,
>> > and tonight I do not have enough concentration to figure out which.  In
>> > any case, this patch works the issue around.
>> 
>> I am low on concentration tonight as well, but this looks right to me.
>> Parse options is expecting the regular old argv and argc. I overlooked
>> this code path during the conversion (though I remember figuring out
>> what this path was doing). Faking the argv and argc a little more
>> accurately, like you do, should work fine.
>
> yes, that's it.
Wait a minute, please.

Why is parse_options() allowed to clobber argv[0] in parse_options_end() in the first place?

I think the memmove() is there to allow the caller to find the remaining arguments after the library parsed out the options in argv[], but wouldn't the caller be expecting to inspect argv[] starting from position 1?

In other words, anything moved to the position of original argv[0] would be lost, and the problem I stumbled upon was exactly that (it triggered because the location of argv[0] was invalid and parse_options_end() wrote into it).

Pierre Habouzit· Jun 9, 2009, 17:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] show-branch: fix segfault when showbranch.default exists

On Tue, Jun 09, 2009 at 09:28:28AM -0700, Junio C Hamano wrote:
Show 29 quoted lines
> Pierre Habouzit <madcoder@madism.org> writes:
> 
> > On Tue, Jun 09, 2009 at 12:17:28AM -0700, Stephen Boyd wrote:
> >> Junio C Hamano wrote:
> >> > I am not sure if this is a bug in parse_options(), or a bug in the caller,
> >> > and tonight I do not have enough concentration to figure out which.  In
> >> > any case, this patch works the issue around.
> >> 
> >> I am low on concentration tonight as well, but this looks right to me.
> >> Parse options is expecting the regular old argv and argc. I overlooked
> >> this code path during the conversion (though I remember figuring out
> >> what this path was doing). Faking the argv and argc a little more
> >> accurately, like you do, should work fine.
> >
> > yes, that's it.
> 
> Wait a minute, please.
> 
> Why is parse_options() allowed to clobber argv[0] in parse_options_end()
> in the first place?
> 
> I think the memmove() is there to allow the caller to find the remaining
> arguments after the library parsed out the options in argv[], but wouldn't
> the caller be expecting to inspect argv[] starting from position 1?
> 
> In other words, anything moved to the position of original argv[0] would
> be lost, and the problem I stumbled upon was exactly that (it triggered
> because the location of argv[0] was invalid and parse_options_end() wrote
> into it).

it does because it supposes that the command is there, and that when I ported the old parsing stuff it's what it was doing.

FWIW I believe it's a misfeature.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Harry Duin· Jun 9, 2009, 17:35 UTC · re: Pierre Habouzit · lore

branch management

I have a few questions on using git. This forum does not seem to address much usage questions, so feel free to direct me to another place, if I am at the wrong address :-)

My questions are on branch management. We are about to switch to git and are figuring out some best practices. We would like to be able to maintain our branch history for a certain period of time (years).

1. We have different repos for doing the integration merging. In
addition to that we have a golden repos, containing what is in
production. The branch history gets pushed by developers to the
integration repos, but not to the golden repos. Since our integration
repos are created for each integration, this means I have lost my
branch history when an integration repos gets deleted.. For now, we
are thinking of having the integrator (an actual person) push the
branch history to another golden repos that will be used to simply
hold all branch history. We could push all branch history to the one
golden repos (that holds production), but it seems like the vast
amount of history might be disadvantageous when we clone repos. Anyone
have a suggestion as to what is the best approach?
2. What command do I use to display only the list of files updated by/for a
branch? This means I want to exclude files that were merged in from say
master - these files are not part of the branch development.
Thanks,
Harry
_______________________________________________________
www.peak6.com

The information in this email or in any file attached hereto is intended only for the personal and confiden- tial use of the individual or entity to which it is addressed and may contain information that is propri- etary and confidential. If you are not the intended recipient of this message you are hereby notified that any review, dissemination, distribution or copying of this message is strictly prohibited. This communica- tion is for information purposes only and should not be regarded as an offer to sell or as a solicitation of an offer to buy any financial product. Email trans- mission cannot be guaranteed to be secure or error- free. P6070214

Alex Riesen· Jun 9, 2009, 19:50 UTC · re: Harry Duin · lore

Re: branch management

Harry Duin, Tue, Jun 09, 2009 19:35:24 +0200:
> My questions are on branch management. 

You seem to think about branches as they are in CVS or SVN (just directories with in-system metadata). You'll find the Git's branching different (being more about history, not at all about directory structure).

Show 6 quoted lines
> 1. We have different repos for doing the integration merging. In
> addition to that we have a golden repos, containing what is in
> production. The branch history gets pushed by developers to the
> integration repos, but not to the golden repos. Since our integration
> repos are created for each integration, this means I have lost my
> branch history when an integration repos gets deleted..

No. As long as there is something, basing its history on the history of the integration repos (and I assume that your "golden", aka "release" or "maintenance" elsewhere, do just that) the history is there. Forever. As it is on every developers machine with a distributed VCS.

> 2. What command do I use to display only the list of files updated by/for a
> branch?

Do you mean the files changed since the branch (presumably "development") diverged from some other branch (presumably "golden")? What for?

> This means I want to exclude files that were merged in from say
> master - these files are not part of the branch development.

You better not exclude files (and while at it, stop thinking about files when working with _history_).

Try describing the problem on _change_ (aka "commit", or point in time or history) level.

P.S. Your (re?)mailer seems to be broken. This mail is not very correct base64 and gmail does not even decode it.

Harry Duin· Jun 10, 2009, 14:02 UTC · re: Alex Riesen · lore

RE: branch management

Yes, I am aware that branching is different in git than what I have used so far with SVN. But apart from the implementation, I have some information that I want to gather about work done on a branch. Here are a few questions/scenarios that I want to make sure we can handle. Remember that our branches are mapped one to one to a Jira ticket.
1. show all code changes performed on a branch (for code review)
2. show list of files/directories touched by a branch (useful when looking for past fixes, but are unsure where the fix was done)
So far I have not found the exact syntax to get this information, but am convinced that git can provide it!
-Harry
-----Original Message-----
From: Alex Riesen [mailto:raa.lkml@gmail.com] 
Sent: Tuesday, June 09, 2009 2:50 PM
To: Harry Duin
Cc: git@vger.kernel.org
Subject: Re: branch management
Harry Duin, Tue, Jun 09, 2009 19:35:24 +0200:
> My questions are on branch management. 

You seem to think about branches as they are in CVS or SVN (just directories with in-system metadata). You'll find the Git's branching different (being more about history, not at all about directory structure).

Show 6 quoted lines
> 1. We have different repos for doing the integration merging. In
> addition to that we have a golden repos, containing what is in
> production. The branch history gets pushed by developers to the
> integration repos, but not to the golden repos. Since our integration
> repos are created for each integration, this means I have lost my
> branch history when an integration repos gets deleted..

No. As long as there is something, basing its history on the history of the integration repos (and I assume that your "golden", aka "release" or "maintenance" elsewhere, do just that) the history is there. Forever. As it is on every developers machine with a distributed VCS.

> 2. What command do I use to display only the list of files updated by/for a
> branch?

Do you mean the files changed since the branch (presumably "development") diverged from some other branch (presumably "golden")? What for?

> This means I want to exclude files that were merged in from say
> master - these files are not part of the branch development.

You better not exclude files (and while at it, stop thinking about files when working with _history_).

Try describing the problem on _change_ (aka "commit", or point in time or history) level.

P.S. Your (re?)mailer seems to be broken. This mail is not very correct base64 and gmail does not even decode it.

_______________________________________________________
www.peak6.com

The information in this email or in any file attached hereto is intended only for the personal and confiden- tial use of the individual or entity to which it is addressed and may contain information that is propri- etary and confidential. If you are not the intended recipient of this message you are hereby notified that any review, dissemination, distribution or copying of this message is strictly prohibited. This communica- tion is for information purposes only and should not be regarded as an offer to sell or as a solicitation of an offer to buy any financial product. Email trans- mission cannot be guaranteed to be secure or error- free. P6070214

Jakub Narebski· Jun 10, 2009, 14:43 UTC · re: Harry Duin · lore

Re: branch management

Harry Duin <hduin@optionshouse.com> writes:
Show 7 quoted lines
> Yes, I am aware that branching is different in git than what I have
> used so far with SVN. But apart from the implementation, I have some
> information that I want to gather about work done on a branch. Here
> are a few questions/scenarios that I want to make sure we can
> handle. Remember that our branches are mapped one to one to a Jira
> ticket.
>

First, the syntax to get all commits in a branch 'branch' which was created ffrom trunk, i.e. branch named 'master' would be

  master..branch

See git-rev-list(1), git-rev-parse(1) and git-log(1) for details of A..B syntax.

> 1. show all code changes performed on a branch (for code review)
$ git log -p master..branch
> 2. show list of files/directories touched by a branch (useful when
>    looking for past fixes, but are unsure where the fix was done)

If you can use pickaxe search (git log -S...), or git-blame, or just looking throught "git log ... -- <path>", you can use

$ git rev-list master..branch | 
  git diff-tree --stdin -r --name-only |
  sort -u
(excluding sha1 hashes).
> 
> So far I have not found the exact syntax to get this information,
> but am convinced that git can provide it!
-- 
Jakub Narebski
Poland
ShadeHawk on #git
Nicolas Pitre· Jun 10, 2009, 15:28 UTC · re: Jakub Narebski · lore

Re: branch management

On Wed, 10 Jun 2009, Jakub Narebski wrote:
Show 11 quoted lines
> Harry Duin <hduin@optionshouse.com> writes:
> 
> > 2. show list of files/directories touched by a branch (useful when
> >    looking for past fixes, but are unsure where the fix was done)
> 
> If you can use pickaxe search (git log -S...), or git-blame, or just
> looking throught "git log ... -- <path>", you can use
> 
> $ git rev-list master..branch | 
>   git diff-tree --stdin -r --name-only |
>   sort -u
What I use in that case is simply
	git diff --stat master...branch
Nicolas
Linus Torvalds· Jun 10, 2009, 17:37 UTC · re: Nicolas Pitre · lore

Re: branch management

On Wed, 10 Jun 2009, Nicolas Pitre wrote:
Show 17 quoted lines
> On Wed, 10 Jun 2009, Jakub Narebski wrote:
> 
> > Harry Duin <hduin@optionshouse.com> writes:
> > 
> > > 2. show list of files/directories touched by a branch (useful when
> > >    looking for past fixes, but are unsure where the fix was done)
> > 
> > If you can use pickaxe search (git log -S...), or git-blame, or just
> > looking throught "git log ... -- <path>", you can use
> > 
> > $ git rev-list master..branch | 
> >   git diff-tree --stdin -r --name-only |
> >   sort -u
> 
> What I use in that case is simply
> 
> 	git diff --stat master...branch

No, that's not going to work in general. The "master...branch" thing works most of the time, but there isn't always a single merge-point, and in the case of criss-cross merges, you'll get it wrong.

It will also hide changes that got reverted (or undone some other way), which can be relevant.

That said, the "git rev-list | git diff-tree" thing has a new name. We call it "git log".

So what Jakub wrote can generally be written as
	git log --name-only --pretty=format:'' master..branch | sort -u

if you're willing to accept the empty line from all the suppressed commit messages (with that "git diff-tree" he'll see all the commit numbers, though, so I guess the 'git log' thing is still better)

			Linus
Daniel Barkalow· Jun 10, 2009, 19:14 UTC · re: Harry Duin · lore

RE: branch management

On Wed, 10 Jun 2009, Harry Duin wrote:
Show 5 quoted lines
> Yes, I am aware that branching is different in git than what I have used 
> so far with SVN. But apart from the implementation, I have some 
> information that I want to gather about work done on a branch. Here are 
> a few questions/scenarios that I want to make sure we can handle. 
> Remember that our branches are mapped one to one to a Jira ticket. 

With respect to the branches that other people see, work in git isn't done "on" a branch, but rather "for" a branch (or more than one branch); it's most literally done "on" a branch on somebody's workstation that doesn't matter to anybody else, and it may be borrowed from some other developer who happens to have done something relevant but not gotten it into production yet. This actually should match your model much better than SVN. I'd say you should have a git repository associated with Jira, where you've got a branch for each ticket. This repository gets maintained along with your ticket database, since it's really not about the history of work on the product but the disposition of bugs in the bug tracker.

> 1. show all code changes performed on a branch (for code review)

Generally, you want "git log master..branch", but note that this only works for code review, not after the fact, because that branch becomes part of master when it gets integrated. If you're looking for "the new code that goes to this ticket", this kind of makes sense, because when the ticket is done, the code isn't new any more.

Also, if you've got a cluster of related tickets, some code may be done that goes towards all of them, and therefore appears new the first time any of them is code reviewed, but becomes part of master before the last ticket is reviewed; so the results here may change (with stuff becoming no longer worth reviewing) after the code is complete but before review is complete.

You may also want to have a branch for "master relative to this ticket", which is master as of the point where the problem was still there (but which includes all of the changes that the developer merged in from other people that aren't part of the work for the ticket). Then you can do "git log base-100..ticket-100" to see the work that contributes to the ticket being resolved that didn't get into master for any other ticket. (That is to say, tickets generally assume that the rest of the product exists and works well enough to get to the point of the particular ticket but not to resolve the ticket; in order to maintain in the future a record of what this reference state was, you need another branch for that, because the future of your product has the ticket resolved and no work at all needed for it).

> 2. show list of files/directories touched by a branch (useful when 
> looking for past fixes, but are unsure where the fix was done) 

"git log --stat ticket-branch" will probably answer your question pretty quickly (maybe requiring looking through several commits, though). If you've got a reference branch from above, you can use "git log --stat base-branch..ticket-branch".

Of course, there's the conceptual issue that maybe somebody else, working on something different, will have done some work that is useful and turns out to be critical to the eventual resolution of the ticket. And it's also likely that other stuff was done that's totally irrelevant. And the difference is not captured electronically at all, but is entirely a matter of whether the developer decided to merge master back into their branch with the goal of getting that particular change or some other change that went gold around the same time.

So it's probably best to do "git log --stat ticket-branch" and keep looking through the results until you find what you're looking for, no matter how it got there.

> So far I have not found the exact syntax to get this information, but am 
> convinced that git can provide it! 

Git can provide all sorts of stuff, but it's also able to provide lots of information that SVN can't provide, and can't necessarily limit itself to telling you things that SVN would be able to capture. So it tells you many things, starting from the most likely relevant and going down to very unlikely (but still vaguely possible).

	-Daniel
*This .sig left intentionally blank*

← back to recent threads