threads / discuss / 4664

What's in git.git

Subject: What's in git.git

## tl;dr

8 messages between Jun 25, 2006 and Jun 27, 2006.

replies: 7people: 5as markdown or json

Junio C Hamano· Jun 25, 2006, 09:37 UTC · lore

No v1.4.1-rc2 this weekend, as I am expecting a visitor today and will mostly be offline. But we got a dozen or so good fixes and cleanups in "master" so far.

In "next", we have the following being cooked. I expect most of them to be in v1.4.1-rc2 sometime next week. Please report breakage on any of these if you see one.

 - "git rebase --merge" updates by Eric Wong.
 - "git diff -b -w" by Johannes.
 - "git cvsimport" multi-branch fixes by Martin and Johannes.
 - "git diff --color" can be controlled from $GIT_DIR/config.
 - "git merge --squash"; this may not be strictly needed as it
   can be emulated with repeated use of "cherry-pick -n" but it
   might be handy in some workflows.

In "pu", I have queued other bigger changes. I do not think most of them are v1.4.1 material yet.

 - "git format-patch --ignore-already-merged" fixes by
   Johannes; I am hoping to have this in v1.4.1.
 - Perl scripts clean-up and Git.xs by Pasky with a few fixes by
   me; in my mailbox there are several other patches in this
   series not in "pu" that primarily makes more scripts to use
   the new Perl infrastructure.  My feeling is that the series
   needs to be proven to have a sound infrastructure (building,
   testing and installation) on a bit wider platforms before
   starting to consider them for inclusion in "next".  We may be
   able to have the basics from this series in v1.4.1, but am
   still uneasy to convert any important scripts to use this,
   even in "next", at this moment.  Not just yet.
 - built-in "git am" by Lukas; it fails some tests which is not
   a good sign, and as I said in a separate message a few days
   ago, I think it is not worth going this route for something
   high-level as "am", so probably the next round I'd drop the
   last patch from the topic.  The patch to clean up cmd_apply()
   might be worth keeping and merging in "next", depending on
   how the Git.xs effort goes, though.
 - "git diff" option clean-ups by Timo Hirvonen; this is moving
   things in a good direction but as with any intrusive cleanups
   still has some rough edges.  I am hoping we can round them
   off soon to merge it in "next".
 - A new PPC SHA-1 implementation by linux@horizon.com; Linus
   showed that this does not make much difference in real life
   from performance point of view.  If it has other benefits
   (such as code size -- which I do not know how it fares), I am
   willing to merge it as it seems to be correct and does not
   seem to introduce regressions.  But I am not a PPC user so
   somebody needs to push my back on this one.
----------------------------------------------------------------
* The 'master' branch has these since the last announcement.
   Jeff King:
      git-commit: allow -e option anywhere on command line
   Johannes Schindelin:
      patch-id: take "commit" prefix as well as "diff-tree" prefix
      apply: replace NO_ACCURATE_DIFF with --inaccurate-eof runtime flag.
   Junio C Hamano:
      Makefile: do not recompile main programs when libraries have changed.
      usage: minimum type fix.
      git-pull: abort when fmt-merge-msg fails.
      diff --color: use reset sequence when we mean reset.
      repo-config: fix printing of bool
   Linus Torvalds:
      Tweak diff colors
   Martin Langhoff:
      git-repack -- respect -q and be quiet
   Matthias Kestenholz:
      add GIT-CFLAGS to .gitignore
   Peter Eriksen:
      Rename safe_strncpy() to strlcpy().
   Petr Baudis:
      Customizable error handlers
   Timo Hirvonen:
      git-merge: Don't use -p when outputting summary
      Clean up diff.c
   Yann Dirson:
      git-commit: filter out log message lines only when editor was run.
* The 'next' branch, in addition, has these.
   Eric Wong:
      rebase: allow --merge option to handle patches merged upstream
      rebase: cleanup rebasing with --merge
      rebase: allow --skip to work with --merge
   Johannes Schindelin:
      Teach diff about -b and -w flags
      cvsimport: always set $ENV{GIT_INDEX_FILE} to $index{$branch}
   Junio C Hamano:
      Makefile: add framework to verify and bench sha1 implementations.
      git-merge --squash
      test-sha1: test hashing large buffer
      diff --color: use $GIT_DIR/config
   Martin Langhoff:
      cvsimport: setup indexes correctly for ancestors and incremental imports
* The 'pu' branch, in addition, has these (this fails the tests).
   Johannes Schindelin:
      add diff_flush_patch_id() to calculate the patch id
      format-patch: introduce "--ignore-if-in-upstream"
   Junio C Hamano:
      Perl interface: add build-time configuration to allow building with -fPIC
      Perl interface: make testsuite work again.
      perl: fix make clean
      Git.pm: tentative fix to test the freshly built Git.pm
   Lukas Sandström:
      Make it possible to call cmd_apply multiple times
      Make git-am a builtin
   Petr Baudis:
      Introduce Git.pm (v4)
      Git.pm: Implement Git::exec_path()
      Git.pm: Call external commands using execv_git_cmd()
      Git.pm: Implement Git::version()
      Add Error.pm to the distribution
      Git.pm: Better error handling
      Git.pm: Handle failed commands' output
      Git.pm: Enhance the command_pipe() mechanism
      Git.pm: Implement options for the command interface
      Git.pm: Add support for subdirectories inside of working copies
      Convert git-mv to use Git.pm
      Git.pm: assorted build related fixes.
   Timo Hirvonen:
      Merge with_raw, with_stat and summary variables to output_format
      Make --raw option available for all diff commands
      Set default diff output format after parsing command line
      DIFF_FORMAT_RAW is not default anymore
      --name-only, --name-status, --check and -s are mutually exclusive
      Remove awkward compatibility warts
   Unknown (linux@horizon.com):
      A better-scheduled PPC SHA-1 implementation.
Linus Torvalds· Jun 25, 2006, 17:47 UTC · re: Junio C Hamano · lore

Re: What's in git.git

On Sun, 25 Jun 2006, Junio C Hamano wrote:
> 
>    Timo Hirvonen:
>       Clean up diff.c
THIS IS CRAP!

Dammit, anybody who claims that casting a constant string to "(char *)" is a _cleanup_ is doing something seriously wrong.

That's crap, crap, crap, CRAP!

If the "cleanup" was about hiding compiler warnings, then dammit, those warnings should be fixed by fixing the code, not by casting the warning away but leaving the broken code.

If the ptr really is never accessed, and doesn't matter, then don't use a constant empty string, use NULL.

And if it _is_ accessed, then casting a constant string to "char *" is _wrong_.

The whole and only point about the "const" warnings is not to hide them, but to fix the code. If you're not going to fix the code, then you shouldn't ask the compiler to warn about it, it's that simple. Adding bogus casts is not the answer.

I really hate how many _bogus_ casts we're growing. Casts are one of the most important features of C (it's what allows you to break the type system if you need to, and turns C into the truly extensible language it is), but they should be used with reverence and care, not to shut up a compiler.

I'm _especially_ disgusted by how this was claimed to be a "cleanup". Adding a cast is _never_ a cleanup.

Dammit, don't do crap like this!
THIS is a cleanup:
-               char *prefix = "";
+               const char *prefix = "";
but THESE are total and utter CRAP:
-               mf->ptr = ""; /* does not matter */
+               mf->ptr = (char *)""; /* does not matter */
-                               s->data = "";
+                               s->data = (char *)"";
and we're better off with the warning than with the new code.

I suspect that both could have been made to use NULL instead to indicate that no pointer exists.

			Linus
Timo Hirvonen· Jun 25, 2006, 18:07 UTC · re: Linus Torvalds · lore

Re: What's in git.git

Linus Torvalds <torvalds@osdl.org> wrote:
Show 11 quoted lines
> On Sun, 25 Jun 2006, Junio C Hamano wrote:
> > 
> >    Timo Hirvonen:
> >       Clean up diff.c
> 
> THIS IS CRAP!
> 
> Dammit, anybody who claims that casting a constant string to "(char *)" is 
> a _cleanup_ is doing something seriously wrong.
> 
> That's crap, crap, crap, CRAP!

Sorry. It got really annoying to skip over the same compiler warnings in vim's quickfix window many times. At that time I just wanted to focus on the code I was writing.

Show 11 quoted lines
> but THESE are total and utter CRAP:
> 
> -               mf->ptr = ""; /* does not matter */
> +               mf->ptr = (char *)""; /* does not matter */
> -                               s->data = "";
> +                               s->data = (char *)"";
> 
> and we're better off with the warning than with the new code.
> 
> I suspect that both could have been made to use NULL instead to indicate 
> that no pointer exists.
I'll look into this.
-- 
http://onion.dynserv.net/~timo/
Linus Torvalds· Jun 25, 2006, 18:43 UTC · re: Timo Hirvonen · lore

Re: What's in git.git

On Sun, 25 Jun 2006, Timo Hirvonen wrote:
Show 5 quoted lines
> > 
> > I suspect that both could have been made to use NULL instead to indicate 
> > that no pointer exists.
> 
> I'll look into this.

An alternative is to really mark the char pointer structure members const. That is often the preferred thing to do, if usage allows it.

The biggest problem I've traditionally had with structure members that are "const char *" has ironically been that the standard C definition of "free()" is misdesigned.

"free()" should take a "const volatile void *", not just a "void *". As it is, you often have to cast things to free() just to avoid the warning about discarding qualifiers. Which is really sad.

It's perfectly valid to do something like
	struct xyzzy {
		const char *member;
		..
	};
	s->member = strdup(string);
	..
	free(s->member);
	free(s)
but sadly, that member free generates a (bogus) warning.

Of course, we should probably do a "xfree()" anyway, to match our "x[c|re]alloc()".

			Linus
Junio C Hamano· Jun 27, 2006, 05:54 UTC · re: Linus Torvalds · lore

Re: What's in git.git

Linus Torvalds <torvalds@osdl.org> writes:
> I suspect that both could have been made to use NULL instead to indicate 
> that no pointer exists.

As long as none of the following does not dereference the NULL pointer that should work fine:

	- memchr(NULL, ch, 0);
        - memcmp(NULL, ptr, 0);
        - stream.next_in = NULL;
          stream.avail_in = 0;
          deflate(&stream, Z_FINISH);
Linus Torvalds· Jun 27, 2006, 06:29 UTC · re: Junio C Hamano · lore

Re: What's in git.git

On Mon, 26 Jun 2006, Junio C Hamano wrote:
Show 11 quoted lines
> 
> As long as none of the following does not dereference the NULL
> pointer that should work fine:
> 
> 	- memchr(NULL, ch, 0);
> 
>         - memcmp(NULL, ptr, 0);
> 
>         - stream.next_in = NULL;
>           stream.avail_in = 0;
>           deflate(&stream, Z_FINISH);
Well, they clearly _shouldn't_, but bugs happen in libraries too.

(The reason they shouldn't is simple: replace the NULL pointer with a perfectly normal pointer at the end of a page, and if you access past the length of the memory area, you'd get the same SIGSEGV that a NULL pointer should give you).

So the question is how much to trust the libraries for a special case that has a clearly "Right Answer", but is probably unusual in practice.

(That said, "malloc(0)" can validly return NULL, so the NULL+<zero length> thing can really happen in real code even if it's not explicit).

If people want to be anally defensive, we should give it a valid pointer. If there are known buggy libraries, I wouldn't object to something explicit like

	#define DUMMYPTR ((void *)"")

and hide it that way - the code may be the exact same thing that I objected to, but now you're showing that you know that the cast is crud, and you're just wanting to pass in some non-NULL valid pointer that can be dereferenced but never written to.

Personally, I think NULL is much better, until somebody shows an actual buggy library.

			Linus
Martin Langhoff· Jun 26, 2006, 22:24 UTC · re: Junio C Hamano · lore

Re: What's in git.git

On 6/25/06, Junio C Hamano <junkio@cox.net> wrote:
>  - "git cvsimport" multi-branch fixes by Martin and Johannes.
Note: I have the nagging suspicion that there's a thinko in the
opening of new branches, but can't get into fixing that till later
today.
cheers,
martin

← back to recent threads