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

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

[Junio C Hamano]

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

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,

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

[Stephen Boyd]

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.

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

[Pierre Habouzit]

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.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

[Junio C Hamano]

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

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

[Pierre Habouzit]

On Tue, Jun 09, 2009 at 09:28:28AM -0700, Junio C Hamano wrote:
> 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

branch management

[Harry Duin]

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

Re: branch management

[Alex Riesen]

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

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

RE: branch management

[Harry Duin]

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

> 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

Re: branch management

[Jakub Narebski]

Harry Duin <hduin@optionshouse.com> writes:

> 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

Re: branch management

[Nicolas Pitre]

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


Nicolas

Re: branch management

[Linus Torvalds]

On Wed, 10 Jun 2009, Nicolas Pitre wrote:

> 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

RE: branch management

[Daniel Barkalow]

On Wed, 10 Jun 2009, Harry Duin wrote:

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