Re: [PATCH] mailmap: fix check for freeing memory

2013-08-20 Thread Thomas Rast 
Stefan Beller stefanbel...@googlemail.com writes:

 The condition as it is written in that line was most likely intended to
 check for the pointer passed to free(), rather than checking for the
 'repo_abbrev', which is already checked against being non null at the
 beginning of the function.
[...]
 - if (repo_abbrev)
 + if (*repo_abbrev)
   free(*repo_abbrev);

But now the test is useless, because free(NULL) is defined to be a
no-op.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mailmap: fix check for freeing memory

2013-08-20 Thread Jeff King 
On Tue, Aug 20, 2013 at 03:40:02PM +0200, Thomas Rast wrote:

 Stefan Beller stefanbel...@googlemail.com writes:
 
  The condition as it is written in that line was most likely intended to
  check for the pointer passed to free(), rather than checking for the
  'repo_abbrev', which is already checked against being non null at the
  beginning of the function.
 [...]
  -   if (repo_abbrev)
  +   if (*repo_abbrev)
  free(*repo_abbrev);
 
 But now the test is useless, because free(NULL) is defined to be a
 no-op.

Yeah, I think we should just drop the conditional completely.

I am not sure of the complete back-story. The earlier check for
repo_abbrev to be non-NULL was added by 8503ee4, after this check on
free() already existed. So that was when this conditional became
redundant.

But the line right after this one unconditionally assigns to
*repo_abbrev, so we would always segfault in such a case anyway (which
is what 8503ee4 was fixing).

So I think it was either a misguided don't pass NULL to free check
that was simply wrong, or it was an incomplete make sure repo_abbrev is
not NULL check. And the first is useless, and the second is now
redundant due to 8503ee4. So it should simply be free().

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mailmap: fix check for freeing memory

2013-08-20 Thread Stefan Beller 
On 08/20/2013 03:40 PM, Thomas Rast wrote:
 Stefan Beller stefanbel...@googlemail.com writes:
 
 The condition as it is written in that line was most likely intended to
 check for the pointer passed to free(), rather than checking for the
 'repo_abbrev', which is already checked against being non null at the
 beginning of the function.
 [...]
 -if (repo_abbrev)
 +if (*repo_abbrev)
  free(*repo_abbrev);
 
 But now the test is useless, because free(NULL) is defined to be a
 no-op.
 

Yes, indeed. Thanks for reviewing.

Stepping two steps back, I am trying to figure out, what this repo_abrev
thing is doing, as I could find no documentation.

It's passed as a double pointer as declared in mailmap.h:
int read_mailmap(struct string_list *map, char **repo_abbrev);

However grepping for read_mailmap( (bracket to prevent finding 
read_mailmap_XXX as often used in mailmap.c itself) 
grep -nHIirF --exclude-dir=.git -- read_mailmap(
throughout all the sources I just find one occurence having the
second argument not being 'NULL' and that is in 
builtin/shortlog.c:212: read_mailmap(log-mailmap, 
log-common_repo_prefix);

which turns out to be:

void shortlog_init(struct shortlog *log)
{
memset(log, 0, sizeof(*log));

read_mailmap(log-mailmap, log-common_repo_prefix);
...

So we're passing there an address, which was just set to zero.
This is the only occurence of passing a value at all and the value
being passed is 0, so the free in the original patch doesn't need 
that check either.

As I am resending the patch, could somebody please explain me
the mechanism of the # repo-abbrev: line? Even git itself doesn't
use it in the .mailmap file, but a quick google search shows up only
kernel repositories.

Stefan




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] mailmap: fix check for freeing memory

2013-08-20 Thread Stefan Beller 
On 08/20/2013 04:23 PM, Thomas Rast wrote:
 Stefan Beller stefanbel...@googlemail.com writes:
 
 As I am resending the patch, could somebody please explain me
 the mechanism of the # repo-abbrev: line? Even git itself doesn't
 use it in the .mailmap file, but a quick google search shows up only
 kernel repositories.
 
 I had no idea (we really lack documentation on this), but some history
 digging shows this:
 
   commit 7595e2ee6ef9b35ebc8dc45543723e1d89765ce3
   Author: Junio C Hamano jun...@cox.net
   Date:   Sat Nov 25 00:07:54 2006 -0800
 
   git-shortlog: make common repository prefix configurable with .mailmap
   
   The code had /pub/scm/linux/kernel/git/ hardcoded which was
   too specific to the kernel project.
   
   With this, a line in the .mailmap file:
   
   # repo-abbrev: /pub/scm/linux/kernel/git/
   
   can be used to cause the substring to be abbreviated to /.../
   on the title line of the commit message.
   
   Signed-off-by: Junio C Hamano jun...@cox.net
 
 It apparently serves to abbreviate commits coming from pull requests,
 with a subject like
 
   Merge branch 'release' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux
 

Thanks for looking through the history, I need to adapt my search patterns. ;)

Personally I have the feel this is a very kernel specific, but also 
useful thing to have. So I would definitely not drop it, but maybe move
it to another place, such as in the .git/config file. 
Then anybody can configure it themselves and maybe even have multiple
abbreviation lines possible.
It's very likely nowadays that there are repos at various different
hosting sites, so just one abbreviation would not cut it anymore.

Or as Jeff just mentioned in his email, it's there for historical
purpose, but not needed anymore as git-merge produces nicer commit
messages there.

As proposed I checked recent kernel history and saw:

$ git log --min-parents=2 --oneline 
d6a5e06 Merge git://git.kernel.org/pub/scm/linux/kernel/git/steve/gfs2-3.0-fixes
7067552 Merge branch 'x86-urgent-for-linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
e91dade Merge branch 'timers-urgent-for-linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
fbf2184 Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux
3387ed8 Merge tag 'drm-intel-fixes-2013-08-15' of 
git://people.freedesktop.org/~danvet/drm-intel
d2b2c08 Merge branch 'drm-fixes-3.11' of 
git://people.freedesktop.org/~agd5f/linux
50e37cc Merge branch 'for-3.11-fixes' of 
git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup
a08797e Merge tag 'ext4_for_linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4
2620bf0 Merge branch 'fixes' of git://git.linaro.org/people/rmk/linux-arm
359d16c Merge branch 'for-3.11' of 
git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
0f7dd1a Merge tag 'clk-fixes-for-linus' of 
git://git.linaro.org/people/mturquette/linux
2d2843e Merge tag 'pm-3.11-rc6' of 
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
f43c606 Merge tag 'sound-3.11' of 
git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
89cb9ae Merge tag 'usb-3.11-rc6' of 
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb

in their .mailmap it read:
# repo-abbrev: /pub/scm/linux/kernel/git/

So the abbreviation doesn't take place, can this feature be turned off?
I'm still confused by that feature.

Thanks,
Stefan





signature.asc
Description: OpenPGP digital signature

Re: [PATCH] mailmap: fix check for freeing memory

2013-08-20 Thread Jeff King 
On Tue, Aug 20, 2013 at 04:38:17PM +0200, Stefan Beller wrote:

 As proposed I checked recent kernel history and saw:
 
 $ git log --min-parents=2 --oneline 
 d6a5e06 Merge 
 git://git.kernel.org/pub/scm/linux/kernel/git/steve/gfs2-3.0-fixes
 7067552 Merge branch 'x86-urgent-for-linus' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
 e91dade Merge branch 'timers-urgent-for-linus' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
 fbf2184 Merge branch 'drm-fixes' of 
 git://people.freedesktop.org/~airlied/linux
 3387ed8 Merge tag 'drm-intel-fixes-2013-08-15' of 
 git://people.freedesktop.org/~danvet/drm-intel
 d2b2c08 Merge branch 'drm-fixes-3.11' of 
 git://people.freedesktop.org/~agd5f/linux
 50e37cc Merge branch 'for-3.11-fixes' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup
 a08797e Merge tag 'ext4_for_linus' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4
 2620bf0 Merge branch 'fixes' of git://git.linaro.org/people/rmk/linux-arm
 359d16c Merge branch 'for-3.11' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k
 0f7dd1a Merge tag 'clk-fixes-for-linus' of 
 git://git.linaro.org/people/mturquette/linux
 2d2843e Merge tag 'pm-3.11-rc6' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
 f43c606 Merge tag 'sound-3.11' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
 89cb9ae Merge tag 'usb-3.11-rc6' of 
 git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
 
 in their .mailmap it read:
   # repo-abbrev: /pub/scm/linux/kernel/git/
 
 So the abbreviation doesn't take place, can this feature be turned off?
 I'm still confused by that feature.

It _only_ impacts git-shortlog, not git-log or other traversals. Making
it an even more dubious feature, IMHO.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mailmap: fix check for freeing memory

2013-08-20 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 It _only_ impacts git-shortlog, not git-log or other traversals. Making
 it an even more dubious feature, IMHO.

I think this was done by an explicit end user request for shortlog.

As you mentioned, merge gives readable merge log messages, but it
deliberately uses the real URL, not your personal nickname for the
remote when writing the title line of a merge, i.e.

Merge [branch x of ]repoURL

so it would be helped by the repository abbreviation.  It probably
was an oversight that we did not extend it to the rest of the log
family.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mailmap: fix check for freeing memory

2013-08-20 Thread Jeff King 
On Tue, Aug 20, 2013 at 10:18:08AM -0700, Junio C Hamano wrote:

 As you mentioned, merge gives readable merge log messages, but it
 deliberately uses the real URL, not your personal nickname for the
 remote when writing the title line of a merge, i.e.
 
   Merge [branch x of ]repoURL
 
 so it would be helped by the repository abbreviation.  It probably
 was an oversight that we did not extend it to the rest of the log
 family.

Ah, yeah, I suppose git-pull will still do that. I was thinking of a
direct git-merge of a tracking branch, which would end up with:

  Merge remote-tracking branch 'origin/master'

whereas git pull origin master in the same case would say:

  Merge branch 'master' of git://github.com/gitster/git

Still, I do not think anybody but the kernel is using it. Most people
simply have shorter URLs that do not need abbreviated (e.g., the GitHub
one shown above). And searching for instances on GitHub yields only the
kernel:

  https://github.com/search?q=repo-abbrev+path%3A.mailmaptype=Code

Anyway, I am not proposing ripping the feature out. It just seems like
it does not have a lot of users, and it is not worth worrying much about
trying to extend it.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html