threads / patch / 3525

patchRe: [PATCH] fmt-merge-msg: avoid open "-|" list form for Perl 5.6

Subject: Re: [PATCH] fmt-merge-msg: avoid open "-|" list form for Perl 5.6

## tl;dr

10 messages between Mar 2, 2006 and Mar 3, 2006. Diffs are folded; open one to read it.

replies: 9people: 6as markdown or json

Christopher Faylor· Mar 2, 2006, 16:44 UTC · lore
So to summarize:

If anyone has a problem with Cygwin where signals do not seem to be working, I'd appreciate a bug report to the Cygwin list. We really do expect that things should work and want to fix things if they don't.

If that isn't possible to use the Cygwin list for some reason, I will continue to read this mailing list and respond to Cygwin problems but I would appreciate it if any Cygwin problem report contained details for reproducing the problem. We usually point people to this page http://cygwin.com/problems.html when they have problems. The basic take away from that page is to provide the cygcheck output which shows what settings have been used for your Cygwin installation. The interesting stuff in that output is the cygwin mount points, the CYGWIN environment variable, and version information about the Cygwin DLL.

The Cygwin web site is http://cygwin.com/ and it has a lot of information about Cygwin. Some of it is undoubtedly out-of-date or unclear but we do try to improve things if they are brought to our attention.

I don't see any reason to respond to this thread any further but I will continue to rectify any misstatements that I see being made about Windows or Cygwin here.

cgf (Cygwin Maintainer)
Shawn Pearce· Mar 2, 2006, 16:55 UTC · re: Christopher Faylor · lore

Maybe I missed this but why are people using the native Windows ActiveState Perl with GIT+Cygwin when Cygwin has a Cygwin-ized Perl installation available?

I've been using the Cygwin Perl with GIT without any problems whatsoever. Including the open(I, "-|")... exec(@argv) code that doesn't work correctly in ActiveState and started this whole thread.

-- 
Shawn.
Alex Riesen· Mar 2, 2006, 22:09 UTC · re: Shawn Pearce · lore
Shawn Pearce, Thu, Mar 02, 2006 17:55:10 +0100:
> Maybe I missed this but why are people using the native Windows
> ActiveState Perl with GIT+Cygwin when Cygwin has a Cygwin-ized Perl
> installation available?

because the people _can't_ use cygwin's perl. There are a lot of reasons mainly: administrative, perl script incompatibilities and cygwin.dll incompatibilities (if you use perl from cygwin, it'll need the correct cygwin.dll. And if a build process uses cygwin tools from, for example, QNX Momentics it often comes to clashes).

> I've been using the Cygwin Perl with GIT without any problems
> whatsoever.  Including the open(I, "-|")... exec(@argv) code that
> doesn't work correctly in ActiveState and started this whole thread.
Unfortunately...
Linus Torvalds· Mar 2, 2006, 23:27 UTC · re: Alex Riesen · lore
On Thu, 2 Mar 2006, Alex Riesen wrote:
Show 7 quoted lines
> Shawn Pearce, Thu, Mar 02, 2006 17:55:10 +0100:
> 
> > I've been using the Cygwin Perl with GIT without any problems
> > whatsoever.  Including the open(I, "-|")... exec(@argv) code that
> > doesn't work correctly in ActiveState and started this whole thread.
> 
> Unfortunately...

Here's a stupid first cut at git-fmt-merge-msg in C using the new revlist library interface.

It's not actually doing exactly the same thing, because I'm a lazy bastard, but some things it does better.

For example, afaik, when merging multiple branches that had partially been merged already (ie they had overlapping new stuff), if I read the old perl code correctly, it would talk about the new stuff multiple times. This one doesn't.

The things it doesn't do:
 - the old one had a limit of 20, the new one has a limit of 10 commits 
   reported
 - the old one was tested, the new one is written by me.
 - the old one honored the "merge.summary" git config option. The new one 
   doesn't.
 - the old one did some formatting of the branch message that I don't 
   follow because I'm not a perl user. The new one just takes the 
   explanatory message for the branch merging as-is.

But hey, this is all part of my cunning plan to make people get involved with the new rev-list libification, by giving them things that _almost_ work, but might need some tweaking.

		Linus
--- snip snip for "fmt-merge-msg.c" snip snip---
/*
 * fmt-merge-msg.c
 *
 * Magic auto-generation of merge messages.
 *
 * Copyright (C) 2006 Linus Torvalds and his army of programming ferrets
 */
#include "cache.h"
#include "commit.h"
#include "revision.h"
static void show_commit(struct commit *commit)
{
	char buffer[256];
	pretty_print_commit(CMIT_FMT_ONELINE, commit, ~0, buffer, sizeof(buffer), 0);
	printf("   * %s\n", buffer);
}
int main(int argc, char **argv)
{
	struct rev_info revs;
	struct commit *commit;
	unsigned char sha1[20];
	char buffer[256];
	setup_revisions(0, NULL, &revs, NULL);
	if (get_sha1("HEAD", sha1) < 0)
		die("no HEAD revision");
	commit = lookup_commit_reference(sha1);
	if (!commit)
		die("no HEAD revision");
	commit->object.flags |= UNINTERESTING;
	insert_by_date(commit, &revs.commits);
	revs.topo_order = 1;
	revs.limited = 1;
	while (fgets(buffer, sizeof(buffer), stdin)) {
		int max;
		char *marker;
		if (get_sha1_hex(buffer, sha1) < 0)
			continue;
		commit = lookup_commit_reference(sha1);
		if (!commit)
			continue;
		/*
		 * Format after the SHA1:
		 *	<tab>marker<tab><type>'<name>' of <src>'
		 *
		 * where string is "not-for-merge" if
		 * we're not interested in this one,
		 * and empty otherwise.
		 */
		marker = buffer + 40;
		if (*marker++ != '\t')
			continue;
		if (*marker++ != '\t')
			continue;
		printf("Merge %s", marker);
		insert_by_date(commit, &revs.commits);
		prepare_revision_walk(&revs);
		max = 10;
		while ((commit = get_revision(&revs)) != NULL) {
			int n = --max;
			if (n > 0)
				show_commit(commit);
			else if (!n)
				printf("   ...");
		}
	}
}
Junio C Hamano· Mar 3, 2006, 00:34 UTC · re: Linus Torvalds · lore
Linus Torvalds <torvalds@osdl.org> writes:
> For example, afaik, when merging multiple branches that had partially been 
> merged already (ie they had overlapping new stuff), if I read the old perl 
> code correctly, it would talk about the new stuff multiple times. This one 
> doesn't.

I think this is not quite right, even though it only matters in Octopus and not many people do Octopus anyway. Suppose you are merging lt/rev-list and fk/blame branches into master, starting from this state:

    ! [master] GIT-VERSION-GEN: squelch unneed
     ! [lt/rev-list] setup_revisions(): handle
      ! [fk/blame] git-blame, take 2
    ---
     +  [lt/rev-list] setup_revisions(): handl
     +  [lt/rev-list^] git-log (internal): mor
     +  [lt/rev-list~2] git-log (internal): ad
      + [fk/blame] git-blame, take 2
      - [fk/blame^] Merge part of 'lt/rev-list
     ++ [lt/rev-list~3] Rip out merge-order an
     ++ [lt/rev-list~4] Tie it all together: "
     ++ [lt/rev-list~5] Introduce trivial new 
     ++ [lt/rev-list~6] git-rev-list libificat
     ++ [lt/rev-list~7] Splitting rev-list int
     ++ [lt/rev-list~8] rev-list split: minimu
     ++ [lt/rev-list~9] First cut at libifying
      + [fk/blame~2] Add git-blame, a tool for
    --- [lt/rev-list~10] Merge branch 'maint' 

And you had lt/rev-list branch first listed in FETCH_HEAD. In this particular example, lt/rev-list has only 3 commits on top of common things, but if your max were 3 instead of 10, the first round would actually show the tip 3 without showing any common stuff, and then the next round to show fk/blame branch would show only the remaining two, without ever showing the common stuff, even though it _could_ say the latest of the common stuff.

> The things it doesn't do:
>  - the old one had a limit of 20, the new one has a limit of 10 commits 
>    reported
Good change I would say, except for the above.
>  - the old one was tested, the new one is written by me.
>  - the old one honored the "merge.summary" git config option. The new one 
>    doesn't.
Easily rectifiable ;-).
>  - the old one did some formatting of the branch message that I don't 
>    follow because I'm not a perl user. The new one just takes the 
>    explanatory message for the branch merging as-is.

FETCH_HEAD has explanatory message in more or less "canonical" form. It has noise word "branch", and the current repository is typically " of .". These are removed by the code, so that you would not have to see:

	Merge branch 'jc/delta' of .
Instead you would see:
	Merge 'jc/delta' into 'next'.

The last part, " into 'next'", is also missing from your version. I can distinguish a merge into 'master' (which does not have " into 'master'") and other branches easily that way, and I find it handy.

Other things the Perl code does are purely for Octopus support: things like coalescing multiple branches taken from the same repositories. You would get something like:

	Merge 'lt/rev-list' and 'fk/blame' into 'next'.
	* lt/rev-list:
	  commit 1
          commit 2
	* fk/blame:
	  commit 3
	  commit 4
instead of (your version):
	Merge branch 'lt/rev-list' of .
	   * commit 1
           * commit 2
	Merge branch 'fk/blame' of .
	   * commit 3
           * commit 4
Linus Torvalds· Mar 3, 2006, 00:49 UTC · re: Junio C Hamano · lore
On Thu, 2 Mar 2006, Junio C Hamano wrote:
Show 9 quoted lines
> 
> And you had lt/rev-list branch first listed in FETCH_HEAD.  In
> this particular example, lt/rev-list has only 3 commits on top
> of common things, but if your max were 3 instead of 10, the
> first round would actually show the tip 3 without showing any
> common stuff, and then the next round to show fk/blame branch
> would show only the remaining two, without ever showing the
> common stuff, even though it _could_ say the latest of the
> common stuff.

Yes. I considered it briefly, and it's fixable, but to fix it you'd have to actualyl walk the parent list yourself, rather than letting get_revision do it all for you.

And what my simple thing shows isn't really technically "wrong", since it has shown that there are commits missing from the output with the "..."

The question is just whether shared commits should be "balanced out", or shown as part of the first branch that merged them. I chose the latter, because it's not only simple, it's unambiguous (any balancing algorithm will depend on some random heuristic or other, and on how many commits are shown.

Show 7 quoted lines
> >  - the old one did some formatting of the branch message that I don't 
> >    follow because I'm not a perl user. The new one just takes the 
> >    explanatory message for the branch merging as-is.
> 
> FETCH_HEAD has explanatory message in more or less "canonical"
> form.  It has noise word "branch", and the current repository is
> typically " of .".

Yeah, I actually looked at a few examples, so I knew what it was basically trying to do, and then I ignored it as not interesting to the exercise, which was to abuse the new revision listing library in interesting ways by calling it multiple times.

		Linus
Junio C Hamano· Mar 3, 2006, 01:25 UTC · re: Linus Torvalds · lore
Linus Torvalds <torvalds@osdl.org> writes:
> Yeah, I actually looked at a few examples, so I knew what it was basically 
> trying to do, and then I ignored it as not interesting to the exercise, 
> which was to abuse the new revision listing library in interesting ways by 
> calling it multiple times.

Abuse is exactly the word. The reason it is an abuse is exactly why you said "... but to fix it you'd have to actualyl walk the parent list yourself, rather than letting get_revision do it all for you." Which relates to the fact that object.c layer is not designed to be used multiple times...

Maybe we want to make object.c layer reusable first?
Linus Torvalds· Mar 3, 2006, 01:52 UTC · re: Junio C Hamano · lore
On Thu, 2 Mar 2006, Junio C Hamano wrote:
Show 15 quoted lines
>
> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > Yeah, I actually looked at a few examples, so I knew what it was basically 
> > trying to do, and then I ignored it as not interesting to the exercise, 
> > which was to abuse the new revision listing library in interesting ways by 
> > calling it multiple times.
> 
> Abuse is exactly the word.  The reason it is an abuse is exactly
> why you said "... but to fix it you'd have to actualyl walk the
> parent list yourself, rather than letting get_revision do it all
> for you."  Which relates to the fact that object.c layer is not
> designed to be used multiple times...
> 
> Maybe we want to make object.c layer reusable first?

No, the "abuse" is actually very much done that way on purpose. It's a bit strange to do "incremental" prepare_revision_walk() calls, but it all comes from the fact that the object structures are "persistent" across the calls, even if we remove them from the list when we walk them.

So it's strange, but that was kind of part of the reason for doing it. It's a _good_ strangeness.

The thing about handling commits that were already in another branch but weren't shown is different: the way to handle that is to generate the _whole_ revision list in one go - instead of incrementally - and then for each branch you merge you show the top 10 "not yet shown" commits.

IOW, that thing would never use "get_revision()" at all, but would instead depend on "prepare_revision_walk()" generating the whole tree, and then you just walk the parent pointers from the branch heads by hand, marking then "seen" as you print them.

So the object layer and the revision parsing actually does exactly the right thing, you just have to decide on how to use them..

			Linus
Christopher Faylor· Mar 3, 2006, 00:14 UTC · re: Alex Riesen · lore
On Thu, Mar 02, 2006 at 11:09:30PM +0100, Alex Riesen wrote:
Show 10 quoted lines
>Shawn Pearce, Thu, Mar 02, 2006 17:55:10 +0100:
>>Maybe I missed this but why are people using the native Windows
>>ActiveState Perl with GIT+Cygwin when Cygwin has a Cygwin-ized Perl
>>installation available?
>
>because the people _can't_ use cygwin's perl.  There are a lot of
>reasons mainly: administrative, perl script incompatibilities and
>cygwin.dll incompatibilities (if you use perl from cygwin, it'll need
>the correct cygwin.dll.  And if a build process uses cygwin tools from,
>for example, QNX Momentics it often comes to clashes).
(Hmm.  I wonder if QNX Momentics is YA GPL violator)

If you have multiple versions of the Cygwin DLL on your system and try to use them all jumbled up together then, yes, you will have problems. This isn't a perl-specific issue. The solution is to put the latest version of your Cygwin DLL in your path (presumably in /bin) and delete all of the older ones.

The newest version is undoubtedly going to be the one downloaded from the Cygwin web site (http://cygwin.com/) but you can get version information from the cygwin DLL by using grep:

  grep -a "^%%% Cygwin" WHEREEVER/cygwin1.dll
if you are not inclined to install the newest version of Cygwin.

I'm sure that there are incompatibilities between ActiveState perl and Cygwin's perl which make it hard to use the same scripts in each so I am not doubting that some people might want to use only ActiveState perl. I don't see how the multiple Cygwin DLL issue can be a problem only for Cygwin perl vs. ActiveState perl.

cgf (who sees a new full-time job looming in the git list)

Johannes Schindelin· Mar 2, 2006, 17:33 UTC · re: Christopher Faylor · lore
Hi,
On Thu, 2 Mar 2006, Christopher Faylor wrote:
> If anyone has a problem with Cygwin where signals do not seem to be
> working, I'd appreciate a bug report to the Cygwin list.  We really do
> expect that things should work and want to fix things if they don't.
I am glad to have you on this list. Thanks for all your efforts.

Ciao, Dscho

← back to recent threads