I'm missing isofs.h

I'm missing isofs.h

[Andrew Morton]

In a current tree, using git-pasky-0.7:

bix:/usr/src/git26> cat .git/tags/v2.6.12-rc3 
a2755a80f40e5794ddc20e00f781af9d6320fafb
bix:/usr/src/git26> git diff -r v2.6.12-rc3|grep isofs.h
+#include "isofs.h"
 #include "zisofs.h"
+#include "isofs.h"
+#include "isofs.h"
+#include "isofs.h"
 #include "zisofs.h"
+#include "isofs.h"
+#include "isofs.h"
+#include "isofs.h"
+#include "isofs.h"


That diff should have included the addition of the new isofs.h, but it
isn't there.

Re: I'm missing isofs.h

[Jan Harkes]

On Tue, Apr 26, 2005 at 09:43:38PM -0700, Andrew Morton wrote:
> In a current tree, using git-pasky-0.7:

It looks like git-pasky-0.7 doesn't include the following commit, but
there are also several other diff and merge related fixes that were
added since then.

Jan


commit 65bc81d6fef619d7aadc5c7116be52860539f17a
tree 9adb399af84228740555d732732983b7a02b019d
parent 93256315b2444601a35484f4fb76cd5723284201
author Petr Baudis <pasky@ucw.cz> Sat, 23 Apr 2005 18:05:07 -0700
committer Linus Torvalds <torvalds@ppc970.osdl.org> Sat, 23 Apr 2005 18:05:07 -0700 

    [PATCH] Fix broken diff-cache output on added files

    Added files were errorneously reported with the - prefix by diff-cache,
    obviously leading to great confusion.

    Signed-off-by: Petr Baudis <pasky@ucw.cz>
    Signed-off-by: Linus Torvalds <torvalds@osdl.org>

Re: I'm missing isofs.h

[Petr Baudis]

Dear diary, on Wed, Apr 27, 2005 at 02:58:44PM CEST, I got a letter
where Jan Harkes <jaharkes@cs.cmu.edu> told me that...
> On Tue, Apr 26, 2005 at 09:43:38PM -0700, Andrew Morton wrote:
> > In a current tree, using git-pasky-0.7:
> 
> It looks like git-pasky-0.7 doesn't include the following commit, but
> there are also several other diff and merge related fixes that were
> added since then.

Why do you think it doesn't include it? I can see the fix in the code.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: I'm missing isofs.h

[Steven Cole]

Andrew Morton wrote:
> In a current tree, using git-pasky-0.7:
> 
> bix:/usr/src/git26> cat .git/tags/v2.6.12-rc3 
> a2755a80f40e5794ddc20e00f781af9d6320fafb
> bix:/usr/src/git26> git diff -r v2.6.12-rc3|grep isofs.h
> +#include "isofs.h"
>  #include "zisofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
>  #include "zisofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
> 
> 
> That diff should have included the addition of the new isofs.h, but it
> isn't there.
> 

I'm seeing unexplained behaviour using the above technique, and I'm
also seeing fs/isofs/isofs.h as missing, along with seven other changes.

I'm using the latest cogito release:
[steven@spc0 COGITO]$ cg-version
cogito-0.8 (3e0fb979cc7541506ec660ab66b83d8120da6d57)

I updated my linux-2.6 repo with cg-update origin, and then created
a current linux-2.6 tree using cg-export.  I diffed that exported
tree with 2.6.12-rc3 and saved the result as "a.diff".

I created another diff using Andrew's technique using cg-diff and saved
that to "b.diff".

I had expected that a.diff and b.diff to be the same, but they are
not, and the AWOL file fs/isofs/isofs.h is among the missing using
Andrew's technique.

Here are the details.

Steven

[steven@spc0 linux-2.6]$ cat .git/HEAD
e8108c98dd6d65613fa0ec9d2300f89c48d554bf

[steven@spc0 linux-2.6]$ fsck-cache --tags
tagged commit a2755a80f40e5794ddc20e00f781af9d6320fafb (v2.6.12-rc3) in 0397236d43e48e821cce5bbe6a80a1a56bb7cc3a
tagged commit 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 (v2.6.12-rc2) in 9e734775f7c22d2f89943ad6c745571f1930105f
expect dangling commits - potential heads - due to lack of head information
dangling commit e8108c98dd6d65613fa0ec9d2300f89c48d554bf

[steven@spc0 linux-2.6]$ cg-export ../linux-2.6-current
[steven@spc0 linux-2.6]$ cg-diff -r v2.6.12-rc3 >../b.diff
[steven@spc0 linux-2.6]$ cd ..

[steven@spc0 COGITO]$ diff -urN linux-2.6.12-rc3 linux-2.6-current >a.diff
#note that linux-2.6.12-rc3 was created by patch from kernel.org.

[steven@spc0 COGITO]$ diffstat a.diff >a.diffstat
[steven@spc0 COGITO]$ diffstat b.diff >b.diffstat
[steven@spc0 COGITO]$ tail -n 1 a.diffstat
  199 files changed, 3083 insertions(+), 1601 deletions(-)
[steven@spc0 COGITO]$ tail -n 1 b.diffstat
  191 files changed, 2539 insertions(+), 1540 deletions(-)
[steven@spc0 COGITO]$ diff -u a.diffstat b.diffstat
--- a.diffstat  2005-04-27 09:07:04.000000000 -0600
+++ b.diffstat  2005-04-27 09:07:14.000000000 -0600
@@ -101,7 +101,6 @@
   drivers/usb/net/zd1201.c                     |   20
   drivers/usb/serial/Kconfig                   |    9
   drivers/usb/serial/Makefile                  |    1
- drivers/usb/serial/hp4x.c                    |   85 +++
   drivers/usb/storage/unusual_devs.h           |   22 -
   drivers/video/imsttfb.c                      |    4
   drivers/video/logo/Kconfig                   |    2
@@ -113,7 +112,6 @@
   fs/isofs/dir.c                               |   13
   fs/isofs/export.c                            |    6
   fs/isofs/inode.c                             |   19
- fs/isofs/isofs.h                             |  190 ++++++++
   fs/isofs/joliet.c                            |    6
   fs/isofs/namei.c                             |   13
   fs/isofs/rock.c                              |    8
@@ -136,15 +134,10 @@
   include/asm-sparc64/pgtable.h                |    5
   include/asm-sparc64/spinlock.h               |   48 +-
   include/linux/iso_fs.h                       |  147 ------
- include/linux/iso_fs_i.h                     |   27 -
- include/linux/iso_fs_sb.h                    |   34 -
   include/linux/netfilter_ipv4.h               |    3
   include/linux/pci_ids.h                      |    1
- include/linux/tc_act/tc_defact.h             |   21
- include/net/act_generic.h                    |  142 ++++++
   include/net/ax25.h                           |   10
   include/net/ipv6.h                           |    2
- include/net/tc_act/tc_defact.h               |   13
   include/net/tcp.h                            |   11
   kernel/panic.c                               |    4
   mm/mempolicy.c                               |    2
@@ -190,11 +183,10 @@
   net/sched/Kconfig                            |   10
   net/sched/Makefile                           |   11
   net/sched/cls_fw.c                           |   31 +
- net/sched/simple.c                           |   93 ++++
   net/unix/af_unix.c                           |    1
   net/xfrm/xfrm_state.c                        |    5
   scripts/mod/file2alias.c                     |  111 ++++-
   security/selinux/hooks.c                     |    3
   sound/oss/msnd_pinnacle.c                    |    2
   sound/ppc/Kconfig                            |    2
- 199 files changed, 3083 insertions(+), 1601 deletions(-)
+ 191 files changed, 2539 insertions(+), 1540 deletions(-)
[steven@spc0 COGITO]$ cg-version
cogito-0.8 (3e0fb979cc7541506ec660ab66b83d8120da6d57)

Re: I'm missing isofs.h

[Jan Harkes]

On Wed, Apr 27, 2005 at 03:58:41PM +0200, Petr Baudis wrote:
> Dear diary, on Wed, Apr 27, 2005 at 02:58:44PM CEST, I got a letter
> where Jan Harkes <jaharkes@cs.cmu.edu> told me that...
> > On Tue, Apr 26, 2005 at 09:43:38PM -0700, Andrew Morton wrote:
> > > In a current tree, using git-pasky-0.7:
> > 
> > It looks like git-pasky-0.7 doesn't include the following commit, but
> > there are also several other diff and merge related fixes that were
> > added since then.
> 
> Why do you think it doesn't include it? I can see the fix in the code.

I looked through the output of cg-log, which I thought had at least some
ordering, and that commit showed up as more recent than the pasky-0.7
entry. It looks like the same change is also part of pasky-0.7, but with
a different commit-id. Sorry about the confusion.

In any case, when I use
    cg-diff -r a2755a80f40e5794ddc20e00f781af9d6320fafb: | grep isofs.h

the missing file does show up,
    ...
    Index: fs/isofs/isofs.h
    +++ fd1621a8c03331bd78abfe52c8c385977d0a9729/fs/isofs/isofs.h (mode:100644 sha1:9ce7b51fb6141ea6b82d85687d490c74755591fb)
    ...

so either I'm missing some subtle command line error (missing ':' after
the tag-id?) or the problem was fixed by some other change. So I looked
through the logs to see if there was anything obvious and the commit I
mentioned looked promising.

Jan

Re: I'm missing isofs.h

[Jan Harkes]

On Wed, Apr 27, 2005 at 12:43:51PM -0400, Jan Harkes wrote:
> In any case, when I use
>     cg-diff -r a2755a80f40e5794ddc20e00f781af9d6320fafb: | grep isofs.h
> 
> the missing file does show up,
>     ...
>     Index: fs/isofs/isofs.h
>     +++ fd1621a8c03331bd78abfe52c8c385977d0a9729/fs/isofs/isofs.h (mode:100644 sha1:9ce7b51fb6141ea6b82d85687d490c74755591fb)
>     ...
> 
> so either I'm missing some subtle command line error (missing ':' after
> the tag-id?)

Looks like that actually is the problem, when I run cg-diff -r, but
leave out the ':' the final output does not include the added isofs.h
file.

Jan

Re: I'm missing isofs.h

[Steven Cole]

Steven Cole wrote:
> Andrew Morton wrote:
> 
>> In a current tree, using git-pasky-0.7:
>>
>> bix:/usr/src/git26> cat .git/tags/v2.6.12-rc3 
>> a2755a80f40e5794ddc20e00f781af9d6320fafb
>> bix:/usr/src/git26> git diff -r v2.6.12-rc3|grep isofs.h
>> +#include "isofs.h"
>>  #include "zisofs.h"
>> +#include "isofs.h"
>> +#include "isofs.h"
>> +#include "isofs.h"
>>  #include "zisofs.h"
>> +#include "isofs.h"
>> +#include "isofs.h"
>> +#include "isofs.h"
>> +#include "isofs.h"
>>
>>
>> That diff should have included the addition of the new isofs.h, but it
>> isn't there.
>>
> 
> I'm seeing unexplained behaviour using the above technique, and I'm
> also seeing fs/isofs/isofs.h as missing, along with seven other changes.
> 

Jan Harkes has found the problem to be a missing ':' at the end of the tag.

Steven

Re: I'm missing isofs.h

[Petr Baudis]

Dear diary, on Wed, Apr 27, 2005 at 06:43:38AM CEST, I got a letter
where Andrew Morton <akpm@osdl.org> told me that...
> In a current tree, using git-pasky-0.7:
> 
> bix:/usr/src/git26> cat .git/tags/v2.6.12-rc3 
> a2755a80f40e5794ddc20e00f781af9d6320fafb
> bix:/usr/src/git26> git diff -r v2.6.12-rc3|grep isofs.h
> +#include "isofs.h"
>  #include "zisofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
>  #include "zisofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
> +#include "isofs.h"
> 
> 
> That diff should have included the addition of the new isofs.h, but it
> isn't there.

Ouch.

Well, using -r v2.6.12-rc3: is a workable workaround, but this is a
problem. With the trailing :, you are diffing against your latest
commit, whilst without the trailing :, you are diffing against your
working tree. ;-)

The problem is in how the latter is implemented:

        export GIT_INDEX_FILE=$(mktemp -t gitdiff.XXXXXX)
        cp .git/index $GIT_INDEX_FILE
        read-tree -m $(tree-id "$id1")
        update-cache --refresh
        tree=$(tree-id "$id1")
	diff-cache -r -z $tree | xargs -0 cg-Xdiffdo ...

So, we are recording our adds to the index cache, but here we use a
different one with the adds are not recorded - so diff-cache won't catch
them.

So I fixed this by doing diff-cache --cached and updating the tmp index
by the +- entries. Pushed out, thanks.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: I'm missing isofs.h

[Linus Torvalds]

On Thu, 28 Apr 2005, Petr Baudis wrote:
> 
> without the trailing :, you are diffing against your
> working tree. ;-)
> 
> The problem is in how the latter is implemented:
> 
>         export GIT_INDEX_FILE=$(mktemp -t gitdiff.XXXXXX)
>         cp .git/index $GIT_INDEX_FILE
>         read-tree -m $(tree-id "$id1")
>         update-cache --refresh
>         tree=$(tree-id "$id1")
> 	diff-cache -r -z $tree | xargs -0 cg-Xdiffdo ...
> 
> So, we are recording our adds to the index cache, but here we use a
> different one with the adds are not recorded - so diff-cache won't catch
> them.

Umm. 

Why do you create the new index file in the first place?

If you're diffing against the current working tree, you should just use 
your current index file, no?

And to get the difference between an old tree and the current working 
tree, you should just need to do

	diff-cache -r -z $tree

and you're done.

In other words, that temporary index file really isn't needed in the 
"diff-cache" world. It can diff the current index against _any_ old tree.

And together with Junio's stuff from today, you can literally just do

	diff-cache -p $tree

and you're done - it diffs any release "$tree" against the current state.

And if you want to diff against the current head (rather than current
working state), a simple

	diff-tree -p $tree $(cat .git/HEAD)

should do it.

			Linus

Cogito nit: cg-update should default to "origin".

[David A. Wheeler]

Minor nit on Cogito: I think cg-update should default to "origin",
not the head, if you leave it unspecified.  Instead, add an
option flag to specify the HEAD.  The origin seems (to me)
to be a MUCH more common situation (and thus the better default).

--- David A. Wheeler

Re: I'm missing isofs.h

[Petr Baudis]

Dear diary, on Thu, Apr 28, 2005 at 02:19:07AM CEST, I got a letter
where Linus Torvalds <torvalds@osdl.org> told me that...
> And to get the difference between an old tree and the current working 
> tree, you should just need to do
> 
> 	diff-cache -r -z $tree
> 
> and you're done.
> 
> In other words, that temporary index file really isn't needed in the 
> "diff-cache" world. It can diff the current index against _any_ old tree.

Oops, you are of course right. This was a stupid leftover from the
pre-diff-cache days, and since then I never looked at this code from
sufficient distance to see it. ;-)

> And together with Junio's stuff from today, you can literally just do
> 
> 	diff-cache -p $tree
> 
> and you're done - it diffs any release "$tree" against the current state.

Actually, I can't; the patch generator is not on par with mine yet.
It does not show modes and does not indicate file adds/removals by
/dev/null - basically, I need something cg-patch can eat (and it should
be backwards compatible). I think throwing the sha1 hashes away will not
harm; I got used to the Index: field and === marker, but I don't care if
I loose it.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: Cogito nit: cg-update should default to "origin".

[Petr Baudis]

Dear diary, on Thu, Apr 28, 2005 at 02:32:32AM CEST, I got a letter
where "David A. Wheeler" <dwheeler@dwheeler.com> told me that...
> Minor nit on Cogito: I think cg-update should default to "origin",
> not the head, if you leave it unspecified.  Instead, add an
> option flag to specify the HEAD.  The origin seems (to me)
> to be a MUCH more common situation (and thus the better default).

Actually, I wasn't too happy with the current update-to-HEAD special
case. Sure, it's similar to SVN, but SVN's concepts are totally
different here, and this special case wart (which does really do
something entirely different than normal cg-update) is one of the
Cogito-related shadows in my mind. What about moving this special case
to something like

	cg-restore

and changing the defaulting of update and pull back to 'origin'? I think
people do this cg-update without arguments so seldom that changing this
now shouldn't hurt much, right?

Another thing is to UI-wise maintain clear difference between cg-cancel
and cg-restore. Do you think the names are distinctive enough? Any
better naming ideas?

Thanks,

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: Cogito nit: cg-update should default to "origin".

[Dan Holmsand]

Petr Baudis wrote:
> Actually, I wasn't too happy with the current update-to-HEAD special
> case. Sure, it's similar to SVN, but SVN's concepts are totally
> different here, and this special case wart (which does really do
> something entirely different than normal cg-update) is one of the
> Cogito-related shadows in my mind. What about moving this special case
> to something like
> 
> 	cg-restore
> 
> and changing the defaulting of update and pull back to 'origin'? I think
> people do this cg-update without arguments so seldom that changing this
> now shouldn't hurt much, right?

How about making the restore thing a special case of cg-cancel instead? 
"Restore deleted files", and "restore deleted and modified files and 
unseek" are similar enough that people will now where to look. Something 
like "cg-cancel -C" (for careful), that only restores deleted files 
would do it, I think.

/dan

Re: I'm missing isofs.h

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> Dear diary, on Thu, Apr 28, 2005 at 02:19:07AM CEST, I got a letter
PB> where Linus Torvalds <torvalds@osdl.org> told me that...
>> And together with Junio's stuff from today, you can literally just do
>> 
>> diff-cache -p $tree
>> 
>> and you're done - it diffs any release "$tree" against the current state.

PB> Actually, I can't; the patch generator is not on par with mine yet.

That's what GIT_EXTERNAL_DIFF is there for.

Re: Cogito nit: cg-update should default to "origin".

[David A. Wheeler]

I said:
>>Minor nit on Cogito: I think cg-update should default to "origin",
>>not the head, if you leave it unspecified. ... The origin seems (to me)
>>to be a MUCH more common situation (and thus the better default).

Petr Baudis replied:
> Actually, I wasn't too happy with the current update-to-HEAD special
> case...

Sounds like we're in agreement! Once the special case goes
away, cg-update in both concept & code essentially becomes:
  cg-pull ${1:-origin} && cg-merge
which has the wonderful advantage of being really, really
easy to explain.  ("cg-update ALWAYS pulls, then merges").

 > I think people do this cg-update without arguments so seldom
 > that changing this now shouldn't hurt much, right?

Absolutely!  Indeed, I find myself doing:
  cg-update {wait for something to happen} {oops} cg-update origin

> What about moving this special case
> to something like
> 	cg-restore
> and changing the defaulting of update and pull back to 'origin'?
...
> Another thing is to UI-wise maintain clear difference between cg-cancel
> and cg-restore. Do you think the names are distinctive enough? Any
> better naming ideas?

Good names for these operations seem to be tough to find.
"cg-cancel" seems odd anyway; you'd think you could
"cancel" a commit and then the commit would stop existing
(not true!).

I looked at a thesaurus; other options to cancel & restore
include: revert, recover, retrieve, reclaim, reclaim, undo.
You could even use the names cg-recover-deleted to recover
deleted files (what cg-update does now without parameters),
and use cg-cancel-edits or cg-cancel-changes to make
clearer commands.  But in the end I have a different idea, hold on...

elsewhere Dan Holmsand said:
 >How about making the restore thing a special case of cg-cancel instead?
 >"Restore deleted files", and "restore deleted and modified files and
 >unseek" are similar enough that people will now where to look.
 >Something like "cg-cancel -C" (for careful), that only restores deleted
 >files would do it, I think.

There's a big risk of not including the "-C" and suddenly losing
everthing.  Since there's NO way to recover these files,
a somewhat safer interface would probably be a better idea.
But merging the concepts may make sense if we can find a single
command name that would help people figure this out.

How about "cg-revert" or "cg-restore"?  The word "revert" is even
in the comments for cg-cancel, but now it makes sense to "revert"
or "restore" the existence of a file (whereas it's really odd
to "cancel" a file deletion).

A serious problem with cg-cancel (and previous cg-undo) is
big data loss, no recovery, of your recent work.... if it's
going to have less & more drastic operations, I'd sure hate
for the drastic operation to be the default.  There's also
missing functionality currently: often I want to revert to the
unedited state for just a single file, or just restore a single file.
So, how about this:

cg-revert [FILE...] or
cg-revert [-d|--deleted]|[-a|--all]
   Reverts some/all files back to the HEAD's state, eliminating changes

   If given a list of 1 or more files, this reverts just the named files
   to the HEAD state. If they were deleted, they are restored;
   if they were edited, their edits are PERMANENTLY LOST.
   If they haven't changed, nothing changes and there is no error.

   If given -d or --deleted, it reverts all deleted files.
   If given -a or --all, it reverts all files
   (everything), resuling in loss of all edits and removals.

How's that for a reasonable UI, replacing both cg-cancel
and cg-update's current no-parameter functionality?

--- David A. Wheeler

Re: I'm missing isofs.h

[Junio C Hamano]

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> Actually, I can't; the patch generator is not on par with mine yet.
PB> It does not show modes and does not indicate file adds/removals by
PB> /dev/null - basically, I need something cg-patch can eat (and it should
PB> be backwards compatible). I think throwing the sha1 hashes away will not
PB> harm; I got used to the Index: field and === marker, but I don't care if
PB> I loose it.

I've looked at what cg-Xdiffdo does.  From the above paragraph,
I sense that it does more than what cg-patch requires, so I took
a look at cg-patch, too.  

Can you help me verify if I understand the requirements cg-patch
has on its input correctly?

 - Follow the convention of showing newly added files with
   "--- /dev/null" and removed files with "+++ /dev/null";

 - Label matches this Perl regexp:

     m|^(---|\+\+\+)\s+[^/]+\/(\S+)\s+.*mode:([0-7]{3,}).*/|

   and you only care about sign ($1), filename ($2) and mode ($3).

To illustrate, cg-Xdiffdo generates something like:

 (modified files)
 --- FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF/fs/ext3/Makefile  (mode:0644)
 +++ EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE/fs/ext3/Makefile  (mode:0664)

 (deleted files)
 --- FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF/fs/ext3/Makefile  (mode:0644)
 +++ /dev/null  (tree:EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE)

 (added files)
 --- /dev/null  (tree:EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE)
 +++ FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF/fs/ext3/Makefile  (mode:0644)

but they could be like the following to satisfy cg-patch:

 (modified files)
 --- a/fs/ext3/Makefile  (mode:0644)
 +++ b/fs/ext3/Makefile  (mode:0664)

 (deleted files)
 --- a/fs/ext3/Makefile  (mode:0644)
 +++ /dev/null

 (added files)
 --- /dev/null
 +++ b/fs/ext3/Makefile  (mode:0644)

Is my understanding correct?  If so it should not be too much
work to generate something like it from within the builtin
stuff.

Provided if that is what the kernel folks can live with (I do
see why the tool wants the mode bits, but it is unusual to see
non-timestamp strings after filenames).

Linus & Andrew, is the above (second) format acceptable for the
kernel work?

[PATCH] Make diff-cache and friends output more cg-patch friendly.

[Junio C Hamano]

This patch changes the way the default arguments to diff are
built when diff-cache and friends are invoked with -p and there
is no GIT_EXTERNAL_DIFF environment variable.  It attempts to be
more cg-patch friendly by:

 - Showing diffs against /dev/null to denote added or removed
   files;

 - Showing file modes for existing files as a comment after the
   diff label.

Unfortunately with this change GIT_DIFF_CMD customization cannot
be supported easily anymore, so it has been dropped.
GIT_DIFF_OPTS customization to change diffs from unified to
context is still there, though.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

diff.c |   56 ++++++++++++++++++++++++++++++++++++--------------------
1 files changed, 36 insertions(+), 20 deletions(-)

# - 04/27 21:50 diff.c clean up temporary file.
# + 04/27 23:18 Attempt to minimally be compatible with cg-Xdiffdo.
--- k/diff.c  (mode:100644)
+++ l/diff.c  (mode:100644)
@@ -7,7 +7,6 @@
 #include "cache.h"
 #include "diff.h"
 
-static char *diff_cmd = "diff -L'k/%s' -L'l/%s'";
 static char *diff_opts = "-pu";
 
 static const char *external_diff(void)
@@ -24,14 +23,12 @@ static const char *external_diff(void)
 	 * alternative styles you can specify via environment
 	 * variables are:
 	 *
-	 * GIT_DIFF_CMD="diff -L '%s' -L '%s'"
 	 * GIT_DIFF_OPTS="-c";
 	 */
 	if (getenv("GIT_EXTERNAL_DIFF"))
 		external_diff_cmd = getenv("GIT_EXTERNAL_DIFF");
 
 	/* In case external diff fails... */
-	diff_cmd = getenv("GIT_DIFF_CMD") ? : diff_cmd;
 	diff_opts = getenv("GIT_DIFF_OPTS") ? : diff_opts;
 
 	done_preparing = 1;
@@ -84,31 +81,50 @@ static struct diff_tempfile {
 static void builtin_diff(const char *name,
 			 struct diff_tempfile *temp)
 {
-	static char *diff_arg  = "'%s' '%s'";
-	const char *name_1_sq = sq_expand(temp[0].name);
-	const char *name_2_sq = sq_expand(temp[1].name);
+	int i, next_at;
+	const char *diff_cmd = "diff -L'%s%s%s' -L'%s%s%s'";
+	const char *diff_arg  = "'%s' '%s'";
+	const char *input_name_sq[2];
+	const char *path0[2];
+	const char *path1[2];
+	char mode[2][20];
 	const char *name_sq = sq_expand(name);
-
-	/* diff_cmd and diff_arg have 4 %s in total which makes
-	 * the sum of these strings 8 bytes larger than required.
+	char *cmd;
+	
+	/* diff_cmd and diff_arg have 8 %s in total which makes
+	 * the sum of these strings 16 bytes larger than required.
 	 * we use 2 spaces around diff-opts, and we need to count
-	 * terminating NUL, so we subtract 5 here.
+	 * terminating NUL, so we subtract 13 here.
 	 */
-	int cmd_size = (strlen(diff_cmd) + 
-			strlen(name_sq) * 2 +
-			strlen(diff_opts) +
-			strlen(diff_arg) +
-			strlen(name_1_sq) + strlen(name_2_sq)
-			- 5);
-	char *cmd = xmalloc(cmd_size);
-	int next_at = 0;
+	int cmd_size = (strlen(diff_cmd) + strlen(diff_opts) +
+			strlen(diff_arg) - 13);
+	for (i = 0; i < 2; i++) {
+		input_name_sq[i] = sq_expand(temp[i].name);
+		if (!strcmp(temp[i].name, "/dev/null")) {
+			path0[i] = "/dev/null";
+			path1[i] = "";
+			mode[i][0] = 0;
+		} else {
+			path0[i] = i ? "l/" : "k/";
+			path1[i] = name_sq;
+			sprintf(mode[i], "  (mode:%s)", temp[i].mode);
+		}
+		cmd_size += (strlen(path0[i]) + strlen(path1[i]) +
+			     strlen(mode[i]) + strlen(input_name_sq[i]));
+	}
+
+	cmd = xmalloc(cmd_size);
 
+	next_at = 0;
 	next_at += snprintf(cmd+next_at, cmd_size-next_at,
-			    diff_cmd, name_sq, name_sq);
+			    diff_cmd,
+			    path0[0], path1[0], mode[0],
+			    path0[1], path1[1], mode[1]);
 	next_at += snprintf(cmd+next_at, cmd_size-next_at,
 			    " %s ", diff_opts);
 	next_at += snprintf(cmd+next_at, cmd_size-next_at,
-			    diff_arg, name_1_sq, name_2_sq);
+			    diff_arg, input_name_sq[0], input_name_sq[1]);
+
 	execlp("/bin/sh","sh", "-c", cmd, NULL);
 }
 

Re: I'm missing isofs.h

[Petr Baudis]

Dear diary, on Thu, Apr 28, 2005 at 07:27:59AM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> PB> Actually, I can't; the patch generator is not on par with mine yet.
> PB> It does not show modes and does not indicate file adds/removals by
> PB> /dev/null - basically, I need something cg-patch can eat (and it should
> PB> be backwards compatible). I think throwing the sha1 hashes away will not
> PB> harm; I got used to the Index: field and === marker, but I don't care if
> PB> I loose it.
> 
> I've looked at what cg-Xdiffdo does.  From the above paragraph,
> I sense that it does more than what cg-patch requires, so I took
> a look at cg-patch, too.  

Yes; that was what the last sentence was about. ;-)

> Can you help me verify if I understand the requirements cg-patch
> has on its input correctly?
> 
>  - Follow the convention of showing newly added files with
>    "--- /dev/null" and removed files with "+++ /dev/null";

Yes.

>  - Label matches this Perl regexp:
> 
>      m|^(---|\+\+\+)\s+[^/]+\/(\S+)\s+.*mode:([0-7]{3,}).*/|
> 
>    and you only care about sign ($1), filename ($2) and mode ($3).

Yes..

>  (modified files)
>  --- a/fs/ext3/Makefile  (mode:0644)
>  +++ b/fs/ext3/Makefile  (mode:0664)
> 
>  (deleted files)
>  --- a/fs/ext3/Makefile  (mode:0644)
>  +++ /dev/null
> 
>  (added files)
>  --- /dev/null
>  +++ b/fs/ext3/Makefile  (mode:0644)
> 
> Is my understanding correct?  If so it should not be too much
> work to generate something like it from within the builtin
> stuff.

Yes, perfectly.

> Provided if that is what the kernel folks can live with (I do
> see why the tool wants the mode bits, but it is unusual to see
> non-timestamp strings after filenames).

There's no reason not to get the timestamps too if you can; just put
them after the attributes. They aren't in the diff now either.

I need the mode bits to set the mode right, surprisingly. :-) Yes, in
part it is a leftover from the old times when we didn't just track the
execute bit; I don't know if it is worth changing this.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor

Re: Cogito nit: cg-update should default to "origin".

[Dan Holmsand]

David A. Wheeler wrote:
> So, how about this:
> 
> cg-revert [FILE...] or
> cg-revert [-d|--deleted]|[-a|--all]
>   Reverts some/all files back to the HEAD's state, eliminating changes

That's very good (and much better than my idea).

/dan

Re: I'm missing isofs.h

[Linus Torvalds]

On Wed, 27 Apr 2005, Junio C Hamano wrote:
> 
> Linus & Andrew, is the above (second) format acceptable for the
> kernel work?

The only thing my stuff needs is that it's "-p1" format, but I don't care 
if the prefix is the sha1 tree-name, or "a/" and "b/" or anything else (I 
think the current thing that the built-in stuff defaults to is a bit 
strange. "k/" and "l/"? I understand "a/" and "b/", and I'd even get "x/" 
and "y/" or "old/" and "new/", but starting counting at "l" is strange ;)

		Linus

Re: I'm missing isofs.h

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> On Wed, 27 Apr 2005, Junio C Hamano wrote:
>> 
>> Linus & Andrew, is the above (second) format acceptable for the
>> kernel work?

LT> The only thing my stuff needs is that it's "-p1" format, ...

Is /dev/null convention OK with you?  I know it is OK for patch
and cg-patch, both of which have built-in knowledge of the
convention, but other tools may get confused.  Here is an
example of diffstat getting confused:

    $ jit-snap -v linus-mirror:0 Makefile comm-z.c
    # - [PATCH] diff-tree -p implies diff-tree -p -r
    # + JIT: indent help text from jit-snap.
    --- k/Makefile  (mode:100644)
    +++ l/Makefile  (mode:100644)
    @@ -7,7 +7,8 @@
     # BREAK YOUR LOCAL DIFFS! show-diff and anything us.....
     # break unless your underlying filesystem supports .....
    ...
    --- /dev/null
    +++ l/comm-z.c  (mode:100644)
    @@ -0,0 +1,101 @@
    +#include <stdio.h>
    +#include <string.h>
    ...

    $ jit-snap -v linus-mirror:0 Makefile comm-z.c | diffstat
    Makefile   |   15 +++++++--
    l/comm-z.c |  101 ++++++++++++++++++++++++++++++++++.....
     2 files changed, 114 insertions(+), 2 deletions(-)

LT> ... but I don't care 
LT> if the prefix is the sha1 tree-name, or "a/" and "b/" or anything else (I 
LT> think the current thing that the built-in stuff defaults to is a bit 
LT> strange. "k/" and "l/"? I understand "a/" and "b/", and I'd even get "x/" 
LT> and "y/" or "old/" and "new/", but starting counting at "l" is strange ;)

I am _very_ glad somebody finally has noticed and voiced the
puzzlement.  It is meant to be a gentle reminder that the tool's
primary purpose is to support development of l-k ;-).

I do not mind if you told me to change them to a/ or b/ (or do
it yourself---it is a single character change in the source) if
you want.

Re: I'm missing isofs.h

[Linus Torvalds]

On Thu, 28 Apr 2005, Junio C Hamano wrote:
> 
> Is /dev/null convention OK with you?

Absolutely. In fact, I prefer it, but I end up just using standard "patch 
-p1" in the end, so..

> Here is an example of diffstat getting confused:

diffstat is _way_ too easily confused by various things. I've seen it
claim "no files" just because the diff had some headers that confused it.  
And yes, you should always tell it to use "-p1" to get the right
pathnames, otherwise it does nonsensical things (if all the diffs happen
to be in "drivers/usb/" it ends up deciding that that's just a common
prefix, and won't actually show it at all).

However, I'm surprised that it's confused by /dev/null. Usually the 
confusion comes from the stuff _after_ the name (ie adding the "mode" etc 
is what I'd have expected to confuse it).

One way to un-confuse diffstat is to add the "Index: " line. I'm not
actually much of a fan of Index: lines myself, and I'd rather somebody
fixed diffstat, but they _do_ work around diffstat problems.

		Linus

Re: I'm missing isofs.h

[David A. Wheeler]

Petr Baudis wrote:
> There's no reason not to get the timestamps too if you can; just put
> them after the attributes. They aren't in the diff now either.
> 
> I need the mode bits to set the mode right, surprisingly. :-) Yes, in
> part it is a leftover from the old times when we didn't just track the
> execute bit; I don't know if it is worth changing this.

Actually, I like having the full mode bits in there.
"git" actually can be useful as a more general capability for
keeping careful track of an entire tree that's NOT just source code
(e.g., your entire home directory tree, so you can replicate it
across machines in its current state).

I can easily imagine an option flag
that stores "modes as they really are", and another that says
"use the modes as they are stored".  Add some support for symlinks,
and you could do quite a bit. Timestamps would be ducky,
too, for the same reason.

--- David A. Wheeler