[PATCH] Cleanup of unused symcache variable inside diff-lib.c

[PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Kjetil Barvik]

Commit c40641b77b0274186fd1b327d5dc3246f814aaaf, 'Optimize
symlink/directory detection' by Linus Torvalds, removed the 'char
*symcache' parameter to the has_symlink_leading_path() function.  This
made all variables currently named 'symcache' inside diff-lib.c
unnecessary.

This also let us throw away the 'struct oneway_unpack_data', and
instead directly use the 'struct rev_info *revs' member, which
was the only member left after removal of the 'symcache[] array'
member.  The 'struct oneway_unpack_data' was introduced by the
following commit:

  948dd346  "diff-files: careful when inspecting work tree items"

Impact: cleanup
        PATH_MAX bytes less memory stack usage in some cases

Signed-off-by: Kjetil Barvik <barvik@broadpark.no>
---
:100644 100644 ae96c64... e6d1d2b... M	diff-lib.c
 diff-lib.c |   40 +++++++++++-----------------------------
 1 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index ae96c64ca209f4df9008198e8a04b160bed618c7..e6d1d2b34147a13aadb5019e0c8336ef5f56ee39 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -61,14 +61,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 	int silent_on_removed = option & DIFF_SILENT_ON_REMOVED;
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
-	char symcache[PATH_MAX];
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
 	if (diff_unmerged_stage < 0)
 		diff_unmerged_stage = 2;
 	entries = active_nr;
-	symcache[0] = '\0';
 	for (i = 0; i < entries; i++) {
 		struct stat st;
 		unsigned int oldmode, newmode;
@@ -198,11 +196,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
  * diff-index
  */
 
-struct oneway_unpack_data {
-	struct rev_info *revs;
-	char symcache[PATH_MAX];
-};
-
 /* A file entry went away or appeared */
 static void diff_index_show_file(struct rev_info *revs,
 				 const char *prefix,
@@ -216,8 +209,7 @@ static void diff_index_show_file(struct rev_info *revs,
 static int get_stat_data(struct cache_entry *ce,
 			 const unsigned char **sha1p,
 			 unsigned int *modep,
-			 int cached, int match_missing,
-			 struct oneway_unpack_data *cbdata)
+			 int cached, int match_missing)
 {
 	const unsigned char *sha1 = ce->sha1;
 	unsigned int mode = ce->ce_mode;
@@ -248,25 +240,24 @@ static int get_stat_data(struct cache_entry *ce,
 	return 0;
 }
 
-static void show_new_file(struct oneway_unpack_data *cbdata,
+static void show_new_file(struct rev_info *revs,
 			  struct cache_entry *new,
 			  int cached, int match_missing)
 {
 	const unsigned char *sha1;
 	unsigned int mode;
-	struct rev_info *revs = cbdata->revs;
 
 	/*
 	 * New file in the index: it might actually be different in
 	 * the working copy.
 	 */
-	if (get_stat_data(new, &sha1, &mode, cached, match_missing, cbdata) < 0)
+	if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0)
 		return;
 
 	diff_index_show_file(revs, "+", new, sha1, mode);
 }
 
-static int show_modified(struct oneway_unpack_data *cbdata,
+static int show_modified(struct rev_info *revs,
 			 struct cache_entry *old,
 			 struct cache_entry *new,
 			 int report_missing,
@@ -274,9 +265,8 @@ static int show_modified(struct oneway_unpack_data *cbdata,
 {
 	unsigned int mode, oldmode;
 	const unsigned char *sha1;
-	struct rev_info *revs = cbdata->revs;
 
-	if (get_stat_data(new, &sha1, &mode, cached, match_missing, cbdata) < 0) {
+	if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0) {
 		if (report_missing)
 			diff_index_show_file(revs, "-", old,
 					     old->sha1, old->ce_mode);
@@ -344,8 +334,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	struct cache_entry *idx,
 	struct cache_entry *tree)
 {
-	struct oneway_unpack_data *cbdata = o->unpack_data;
-	struct rev_info *revs = cbdata->revs;
+	struct rev_info *revs = o->unpack_data;;
 	int match_missing, cached;
 
 	/*
@@ -368,7 +357,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	 * Something added to the tree?
 	 */
 	if (!tree) {
-		show_new_file(cbdata, idx, cached, match_missing);
+		show_new_file(revs, idx, cached, match_missing);
 		return;
 	}
 
@@ -381,7 +370,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	}
 
 	/* Show difference between old and new */
-	show_modified(cbdata, tree, idx, 1, cached, match_missing);
+	show_modified(revs, tree, idx, 1, cached, match_missing);
 }
 
 static inline void skip_same_name(struct cache_entry *ce, struct unpack_trees_options *o)
@@ -418,8 +407,7 @@ static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)
 {
 	struct cache_entry *idx = src[0];
 	struct cache_entry *tree = src[1];
-	struct oneway_unpack_data *cbdata = o->unpack_data;
-	struct rev_info *revs = cbdata->revs;
+	struct rev_info *revs = o->unpack_data;
 
 	if (idx && ce_stage(idx))
 		skip_same_name(idx, o);
@@ -446,7 +434,6 @@ int run_diff_index(struct rev_info *revs, int cached)
 	const char *tree_name;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
-	struct oneway_unpack_data unpack_cb;
 
 	mark_merge_entries();
 
@@ -456,14 +443,12 @@ int run_diff_index(struct rev_info *revs, int cached)
 	if (!tree)
 		return error("bad tree object %s", tree_name);
 
-	unpack_cb.revs = revs;
-	unpack_cb.symcache[0] = '\0';
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 1;
 	opts.index_only = cached;
 	opts.merge = 1;
 	opts.fn = oneway_diff;
-	opts.unpack_data = &unpack_cb;
+	opts.unpack_data = revs;
 	opts.src_index = &the_index;
 	opts.dst_index = NULL;
 
@@ -486,7 +471,6 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	struct cache_entry *last = NULL;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
-	struct oneway_unpack_data unpack_cb;
 
 	/*
 	 * This is used by git-blame to run diff-cache internally;
@@ -515,14 +499,12 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	if (!tree)
 		die("bad tree object %s", sha1_to_hex(tree_sha1));
 
-	unpack_cb.revs = &revs;
-	unpack_cb.symcache[0] = '\0';
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 1;
 	opts.index_only = 1;
 	opts.merge = 1;
 	opts.fn = oneway_diff;
-	opts.unpack_data = &unpack_cb;
+	opts.unpack_data = &revs;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 
-- 
1.6.1.rc1.49.g7f705

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Johannes Schindelin]

Hi,

On Sun, 11 Jan 2009, Kjetil Barvik wrote:

> ---
> :100644 100644 ae96c64... e6d1d2b... M	diff-lib.c

I wonder what that line is all about, since ...

>  diff-lib.c |   40 +++++++++++-----------------------------
>  1 files changed, 11 insertions(+), 29 deletions(-)
> 
> diff --git a/diff-lib.c b/diff-lib.c
> index ae96c64ca209f4df9008198e8a04b160bed618c7..e6d1d2b34147a13aadb5019e0c8336ef5f56ee39 100644

... we have the information right there already.

Ciao,
Dscho

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Kjetil Barvik]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Sun, 11 Jan 2009, Kjetil Barvik wrote:
>
>> ---
>> :100644 100644 ae96c64... e6d1d2b... M	diff-lib.c
>
> I wonder what that line is all about, since ...
>
>>  diff-lib.c |   40 +++++++++++-----------------------------
>>  1 files changed, 11 insertions(+), 29 deletions(-)
>> 
>> diff --git a/diff-lib.c b/diff-lib.c
>> index ae96c64ca209f4df9008198e8a04b160bed618c7..e6d1d2b34147a13aadb5019e0c8336ef5f56ee39 100644
>
> ... we have the information right there already.
>
> Ciao,
> Dscho

  hmmm, I tried the following commands

jetil@localhost ~/git/git $ git pull   
Already up-to-date.
kjetil@localhost ~/git/git $ git checkout -q my_origin_maint && grep symcache *.c
diff-lib.c:	char symcache[PATH_MAX];
diff-lib.c:	symcache[0] = '\0';
diff-lib.c:	char symcache[PATH_MAX];
diff-lib.c:	unpack_cb.symcache[0] = '\0';
diff-lib.c:	unpack_cb.symcache[0] = '\0';
kjetil@localhost ~/git/git $ git checkout -q my_origin_master && grep symcache *.c
diff-lib.c:	char symcache[PATH_MAX];
diff-lib.c:	symcache[0] = '\0';
diff-lib.c:	char symcache[PATH_MAX];
diff-lib.c:	unpack_cb.symcache[0] = '\0';
diff-lib.c:	unpack_cb.symcache[0] = '\0';
kjetil@localhost ~/git/git $ git checkout -q my_origin_next && grep symcache *.c
diff-lib.c:	char symcache[PATH_MAX];
diff-lib.c:	symcache[0] = '\0';
diff-lib.c:	char symcache[PATH_MAX];
diff-lib.c:	unpack_cb.symcache[0] = '\0';
diff-lib.c:	unpack_cb.symcache[0] = '\0';
kjetil@localhost ~/git/git $ git checkout -q my_origin_pu && grep symcache *.c
diff-lib.c:	char symcache[PATH_MAX];
diff-lib.c:	symcache[0] = '\0';
diff-lib.c:	char symcache[PATH_MAX];
diff-lib.c:	unpack_cb.symcache[0] = '\0';
diff-lib.c:	unpack_cb.symcache[0] = '\0';
kjetil@localhost ~/git/git $ cd 
kjetil@localhost ~ $ mkdir git2
kjetil@localhost ~ $ cd git2                                          
kjetil@localhost ~/git2 $ git clone -q git://git.kernel.org/pub/scm/git/git.git
kjetil@localhost ~/git2 $ cd git/
kjetil@localhost ~/git2/git $ git show e6d1d2b34147a13aadb5019e0c8336ef5f56ee39
     outputs => 
            fatal: bad object e6d1d2b34147a13aadb5019e0c8336ef5f56ee39
kjetil@localhost ~/git2/git $ git show e6d1d2b
     outputs => 
            fatal: ambiguous argument 'e6d1d2b': unknown revision or path not in the working tree.
            Use '--' to separate paths from revisions
kjetil@localhost ~/git2/git $ git show -- e6d1d2b
     outputs => (nothing)

  Can you explain to a new git user?  As far as I can tell, I do not see
  that this patch is included in the public awailable git tree.  Where
  should the patch be?  I can not see it...  help!  

  -- kjetil

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Johannes Schindelin]

Hi,

On Sun, 11 Jan 2009, Kjetil Barvik wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 11 Jan 2009, Kjetil Barvik wrote:
> >
> >> :100644 100644 ae96c64... e6d1d2b... M	diff-lib.c
>
> [...]
>
> kjetil@localhost ~/git2 $ git clone -q git://git.kernel.org/pub/scm/git/git.git
> kjetil@localhost ~/git2 $ cd git/
> kjetil@localhost ~/git2/git $ git show e6d1d2b34147a13aadb5019e0c8336ef5f56ee39
>      outputs => 
>             fatal: bad object e6d1d2b34147a13aadb5019e0c8336ef5f56ee39

Your patch has not been applied yet.  So no surprise there: your version 
of diff-lib.c is not there.  You'll have more luck with ae96c64, I 
guess...

My question was more: why do you do additional work and put a git diff 
--raw between the commit message and the diffstat when that information is 
in the patch already?

Ciao,
Dscho

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Kjetil Barvik]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
<snipp>
> My question was more: why do you do additional work and put a git diff 
> --raw between the commit message and the diffstat when that information is 
> in the patch already?

 Ok, I see.  I (re)used the 'git format-patch' command from previous
 run, and this time it was (without the line-breaks):

    git format-patch  --stat  --patch-with-raw -1 --signoff -M -C
       --summary --full-index --subject-prefix="PATCH"
       --output-directory ../diff_lib_c_symcache_cleanup_v1/

  Regarding the '--summary' I think it is a nice thing to do (so I try
  to always use it), and the '--full-index' is such that the git software
  have a less chance of producing a collision.

  -- kjetil

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Johannes Schindelin]

Hi,

On Sun, 11 Jan 2009, Kjetil Barvik wrote:

>     git format-patch

Necessary.

>  --stat

Not necessary.

>  --patch-with-raw

As I showed you, also not necessary.

> -1

Necessary.

> --signoff

If you had signed off on your commits already, not necessary.  (FWIW I 
keep a sign off as a marker that I actually think this is good to be 
submitted, or at least close.)

> -M -C

Usualy not necessary, unless you rename some file, or introduce a huge 
code 
duplication.

>        --summary

Dunno.  I think it's not really necessary, you see that from the diff.

> --full-index

I find it makes the patch hard to read, as the index line will always 
wrap.  And it's not necessary, as it is so highly unlikely that the hash 
is unique in your repository, but not mine.

> --subject-prefix="PATCH"

Not necessary.

>        --output-directory ../diff_lib_c_symcache_cleanup_v1/

If you insist...

Sure, you can make it complicated, but I usually prefer something like

	$ git format-patch -3 --cover-letter

Nice 'n easy.

Ciao,
Dscho

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Kjetil Barvik]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

<snipp>
>> --signoff
>
> If you had signed off on your commits already, not necessary.  (FWIW I 
> keep a sign off as a marker that I actually think this is good to be 
> submitted, or at least close.)

  From the Documentation/SubmittingPatches file, for me I think that the
  '-signoff' is supposed to mean:

  [....]
   - if you want your work included in git.git, add a "Signed-off-by:
     Your Name <you@example.com>" line to the commit message (or just
     use the option "-s" when committing) to confirm that you agree to
     the Developer's Certificate of Origin
  [....]
  The sign-off is a simple line at the end of the explanation for the
  patch, which certifies that you wrote it or otherwise have the right
  to pass it on as a open-source patch.  The rules are pretty simple: if
  you can certify the below:

        Developer's Certificate of Origin 1.1

        By making a contribution to this project, I certify that:
  [....]
  then you just add a line saying

	Signed-off-by: Random J Developer <random@developer.example.org>

  This line can be automatically added by git if you run the git-commit
  command with the -s option.
  [....]
  
  And the 'a' and the 'd' in the DCO I do agree with in this particular
  situation, so I added a '--signoff' to the patches.

  -- kjetil

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

>   And the 'a' and the 'd' in the DCO I do agree with in this particular
>   situation, so I added a '--signoff' to the patches.

I think Dscho's suggestion was to sign-off when you commit, not when you
format-patch.  It won't make any difference either way to me nor other
people who reads the list, because nobody can tell which way you used by
looking at your e-mail, but it is a good habit to get into if you work on
git or the kernel.

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> <snipp>
>> My question was more: why do you do additional work and put a git diff 
>> --raw between the commit message and the diffstat when that information is 
>> in the patch already?
>
>  Ok, I see.  I (re)used the 'git format-patch' command from previous
>  run, and this time it was (without the line-breaks):
>
>     git format-patch  --stat  --patch-with-raw -1 --signoff -M -C
>        --summary --full-index --subject-prefix="PATCH"
>        --output-directory ../diff_lib_c_symcache_cleanup_v1/

Please drop --patch-with-raw and --full-index.  They are distracting.

I do not think using --subject-prefix=PATCH to repeat what is default adds
any value either.

Re: [PATCH] Cleanup of unused symcache variable inside diff-lib.c

[Junio C Hamano]

Kjetil Barvik <barvik@broadpark.no> writes:

> diff --git a/diff-lib.c b/diff-lib.c
> index ae96c64ca209f4df9008198e8a04b160bed618c7..e6d1d2b34147a13aadb5019e0c8336ef5f56ee39 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> ...
> @@ -344,8 +334,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
>  	struct cache_entry *idx,
>  	struct cache_entry *tree)
>  {
> -	struct oneway_unpack_data *cbdata = o->unpack_data;
> -	struct rev_info *revs = cbdata->revs;
> +	struct rev_info *revs = o->unpack_data;;

Thanks; I'll clean-up the extra semicolon and apply.