threads / patch / 17090

patchCleanup of unused symcache variable inside diff-lib.c

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

## tl;dr

10 messages between Jan 11, 2009 and Jan 12, 2009. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Kjetil Barvik· Jan 11, 2009, 18:36 UTC · lore

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(-)
Show changes to diff-lib.c +11 −29
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
Johannes Schindelin· Jan 11, 2009, 18:45 UTC · re: Kjetil Barvik · lore

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

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 ...
Show 5 quoted lines
>  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

Kjetil Barvik· Jan 11, 2009, 19:32 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 19 quoted lines
> 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
Johannes Schindelin· Jan 11, 2009, 19:46 UTC · re: Kjetil Barvik · lore

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

Hi,
On Sun, 11 Jan 2009, Kjetil Barvik wrote:
Show 13 quoted lines
> 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

Kjetil Barvik· Jan 11, 2009, 20:09 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Jan 11, 2009, 20:17 UTC · re: Kjetil Barvik · lore

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

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

Kjetil Barvik· Jan 11, 2009, 20:50 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
<snipp>
Show 5 quoted lines
>> --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
Junio C Hamano· Jan 11, 2009, 22:25 UTC · re: Kjetil Barvik · lore

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

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.

Junio C Hamano· Jan 11, 2009, 22:23 UTC · re: Kjetil Barvik · lore

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

Kjetil Barvik <barvik@broadpark.no> writes:
Show 12 quoted lines
> 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.

Junio C Hamano· Jan 12, 2009, 05:39 UTC · re: Kjetil Barvik · lore

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

Kjetil Barvik <barvik@broadpark.no> writes:
Show 12 quoted lines
> 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.

← back to recent threads