split index: sparse warning

split index: sparse warning

[Ramsay Jones]

Hi Duy,

I haven't looked at your split-index series at all. However, sparse has
nagged me to take a quick squint at the result of the series as it
appears in the 'pu' branch.

In particular, sparse complains thus:

      SP sequencer.c
  sequencer.c:690:49: error: incompatible types for operation (>=)
  sequencer.c:690:49:    left side has type int ( extern [toplevel] *<noident> )( ... )
  sequencer.c:690:49:    right side has type int

... which is fair enough; index_fd is, indeed, a function (pointer) and
not an int file descriptor! The offending code looks like:

683 static void read_and_refresh_cache(struct replay_opts *opts)
684 {
685         static struct lock_file index_lock;
686         hold_locked_index(&index_lock, 0);
687         if (read_index_preload(&the_index, NULL) < 0)
688                 die(_("git %s: failed to read the index"), action_name(opts));
689         refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
690         if (the_index.cache_changed && index_fd >= 0) {
691                 if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
692                         die(_("git %s: failed to refresh the index"), action_name(opts));
693         }
694         rollback_lock_file(&index_lock);
695 }


It seems that, in an earlier commit (33c297aa), index_fd was declared
as a local int variable (hiding the global function) which was then
initialised by a call to hold_locked_index().

I assume that the conditional should be changed to something like:

690         if (the_index.cache_changed && index_lock.fd >= 0) {

... but I haven't spent any time investigating this, so take this
suggestion which a large pinch of salt! :-P

ATB,
Ramsay Jones

Re: split index: sparse warning

[Duy Nguyen]

On Thu, May 1, 2014 at 6:44 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote:
> Hi Duy,
>
> I haven't looked at your split-index series at all. However, sparse has
> nagged me to take a quick squint at the result of the series as it
> appears in the 'pu' branch.
>
> In particular, sparse complains thus:
>
>       SP sequencer.c
>   sequencer.c:690:49: error: incompatible types for operation (>=)
>   sequencer.c:690:49:    left side has type int ( extern [toplevel] *<noident> )( ... )
>   sequencer.c:690:49:    right side has type int
>
> ... which is fair enough; index_fd is, indeed, a function (pointer) and
> not an int file descriptor! The offending code looks like:
>
> 683 static void read_and_refresh_cache(struct replay_opts *opts)
> 684 {
> 685         static struct lock_file index_lock;
> 686         hold_locked_index(&index_lock, 0);
> 687         if (read_index_preload(&the_index, NULL) < 0)
> 688                 die(_("git %s: failed to read the index"), action_name(opts));
> 689         refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
> 690         if (the_index.cache_changed && index_fd >= 0) {
> 691                 if (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> 692                         die(_("git %s: failed to refresh the index"), action_name(opts));
> 693         }
> 694         rollback_lock_file(&index_lock);
> 695 }
>
>
> It seems that, in an earlier commit (33c297aa), index_fd was declared
> as a local int variable (hiding the global function) which was then
> initialised by a call to hold_locked_index().

Ugh.. after removing index_fd the local variable, index_fd now becomes
the function from sha1_file.c. Noted and will fix it in the next
reroll.
--
Duy

[PATCH nd/split-index] fixup! read-cache: new API write_locked_index instead of write_index/write_cache

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I intended it resend the series after the comments I received, but it
 looks like Junio has picked up all comments except this one, so
 here's the fix.

 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 377c877..4b709db 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -672,7 +672,7 @@ static void prepare_revs(struct replay_opts *opts)
 static void read_and_refresh_cache(struct replay_opts *opts)
 {
 	static struct lock_file index_lock;
-	hold_locked_index(&index_lock, 0);
+	int index_fd = hold_locked_index(&index_lock, 0);
 	if (read_index_preload(&the_index, NULL) < 0)
 		die(_("git %s: failed to read the index"), action_name(opts));
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
-- 
1.9.1.346.ga2b5940

Re: [PATCH nd/split-index] fixup! read-cache: new API write_locked_index instead of write_index/write_cache

[Ramsay Jones]

On 01/06/14 01:47, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  I intended it resend the series after the comments I received, but it
>  looks like Junio has picked up all comments except this one, so
>  here's the fix.
> 
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 377c877..4b709db 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -672,7 +672,7 @@ static void prepare_revs(struct replay_opts *opts)
>  static void read_and_refresh_cache(struct replay_opts *opts)
>  {
>  	static struct lock_file index_lock;
> -	hold_locked_index(&index_lock, 0);
> +	int index_fd = hold_locked_index(&index_lock, 0);
>  	if (read_index_preload(&the_index, NULL) < 0)
>  		die(_("git %s: failed to read the index"), action_name(opts));
>  	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
> 

Yep, this silences sparse. I would have declared the variable to be
(say) fd (and changed the _use_ site as well, of course!), rather
than once again hiding the index_fd() global function. However, this
is perfectly fine as-is.

The actual culprit is the index_fd() global function, but renaming it
now is probably not worth the code churn (and I'm lazy). ;-)

Thanks!

ATB,
Ramsay Jones