Re: [PATCH 3/4] dir: introduce file_size() to check the size of file
Hey Torsten,
On Sun, Jun 12, 2016 at 4:14 PM, Torsten Bögershausen wrote:
>>> So what I understand, you want something like this:
>>>
>>> +ssize_t file_size_not_zero(const char *filename)
>>> +{
>>> + struct stat st;
>>> + if (stat(filename, &st) < 0)
>>> + return -1;
>>> + return !!st.st_size);
>>> +}
>>
>> For the purpose of bisect_reset(), Yes. BTW a similar function exist
>> in builtin/am.c with the name is_empty_file(). But as Christian points
>> out file_size() could help to refactor other parts of code.
>>
>
> Please allow one or more late comments:
That's perfectly fine.
> If is_empty_file() does what you need, then it can be moved into wrapper.c
> and simply be re-used in your code.
Thanks for informing. I was unaware about the use of wrapper.c
> If you want to introduce a new function, that can be used for other
> refactoring,
> then the whole thing would ideally go into a single commit,
> or into a single series.
> That may probably be out of the scope for your current efforts ?
On re-thinking, I think introducing file_size() is out of the scope
for the current efforts and I will stick to is_empty_file(). Will move
it to wrapper.c and then use it in my code. I am not sure but I think
a few other parts could also use is_empty_file(). I will check on that
probably after GSoC as a cleanup.
> What really makes me concern is the mixture of signed - and unsigned:
> ssize_t file_size(const char *filename)
> +{
> + struct stat st;
> + if (stat(filename, &st) < 0)
> + return -1;
> + return xsize_t(st.st_size);
> +}
>
> To my understanding a file size is either 0, or a positive integer.
> Returning -1 is of course impossible with a positive integer.
True.
> So either the function is changed like this:
>
> int file_size(const char *filename, size_t *len)
> +{
> + struct stat st;
> + if (stat(filename, &st) < 0)
> + return -1;
> + *len = xsize_t(st.st_size);
> + return 0;
> +}
>
> Or, if that works for you:
>
> size_t file_size(const char *filename)
> +{
> + struct stat st;
> + if (stat(filename, &st) < 0)
> + return 0;
> + return xsize_t(st.st_size);
> +}
>
> Or, more git-ish:
>
> size_t file_size(const char *filename)
> +{
> + struct stat st;
> + if (stat(filename, &st))
> + return 0;
> + return xsize_t(st.st_size);
> +}
>
> (And then builtin/am.c can be changed to use the new function.
I think I will just skip file_size() for now.
Thanks for your comments!
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] dir: introduce file_size() to check the size of file
>> So what I understand, you want something like this:
>>
>> +ssize_t file_size_not_zero(const char *filename)
>> +{
>> + struct stat st;
>> + if (stat(filename, &st) < 0)
>> + return -1;
>> + return !!st.st_size);
>> +}
>
> For the purpose of bisect_reset(), Yes. BTW a similar function exist
> in builtin/am.c with the name is_empty_file(). But as Christian points
> out file_size() could help to refactor other parts of code.
>
Please allow one or more late comments:
If is_empty_file() does what you need, then it can be moved into wrapper.c
and simply be re-used in your code.
If you want to introduce a new function, that can be used for other refactoring,
then the whole thing would ideally go into a single commit,
or into a single series.
That may probably be out of the scope for your current efforts ?
What really makes me concern is the mixture of signed - and unsigned:
ssize_t file_size(const char *filename)
+{
+ struct stat st;
+ if (stat(filename, &st) < 0)
+ return -1;
+ return xsize_t(st.st_size);
+}
To my understanding a file size is either 0, or a positive integer.
Returning -1 is of course impossible with a positive integer.
So either the function is changed like this:
int file_size(const char *filename, size_t *len)
+{
+ struct stat st;
+ if (stat(filename, &st) < 0)
+ return -1;
+ *len = xsize_t(st.st_size);
+ return 0;
+}
Or, if that works for you:
size_t file_size(const char *filename)
+{
+ struct stat st;
+ if (stat(filename, &st) < 0)
+ return 0;
+ return xsize_t(st.st_size);
+}
Or, more git-ish:
size_t file_size(const char *filename)
+{
+ struct stat st;
+ if (stat(filename, &st))
+ return 0;
+ return xsize_t(st.st_size);
+}
(And then builtin/am.c can be changed to use the new function.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] dir: introduce file_size() to check the size of file
Hey Torsten,
On Wed, Jun 8, 2016 at 1:47 PM, Torsten Bögershausen wrote:
> On 06/08/2016 09:57 AM, Pranit Bauva wrote:
>>
>> Hey Eric,
>>
>> On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine
>> wrote:
>>>
>>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva
>>> wrote:
dir: introduce file_size() to check the size of file
At times we require to see if the file is empty and get the size of the
file. By using stat we can get the file size without actually having to
open the file to check for its contents.
>>>
>>> The sole caller of this function in patch 4/4 does so only to check if
>>> the file exists; it doesn't even care about the file's size, thus
>>> neither this function nor this patch seem justified and probably ought
>>> to be dropped unless some better and stronger justification can be
>>> shown.
>>
>> Umm, actually there is a subtle difference between file_exists() and
>> file_size(). file_exists() *only* checks whether the file exists or
>> not while file_size() can also be used to check whether the file is
>> empty or not also see the implementation of both of them which shows
>> the difference. In fact it doesn't care at all about the file size.
>> Now there are a lot of instances in shell scripts where there are
>> quite some differences with -f and -s and some places *do care* about
>> this subtle difference. For eg. in bisect_reset() we test whether the
>> file .git/BISECT_START has some contents in it. But I guess I can add
>> some more justification in the commit message. What do you think?
>>
Signed-off-by: Pranit Bauva
---
diff --git a/dir.c b/dir.c
@@ -2036,6 +2036,14 @@ int file_exists(const char *f)
+ssize_t file_size(const char *filename)
+{
+ struct stat st;
+ if (stat(filename, &st) < 0)
+ return -1;
+ return xsize_t(st.st_size);
+}
+
diff --git a/dir.h b/dir.h
@@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list
*el);
+/*
+ * Return the size of the file `filename`. It returns -1 if error
+ * occurred, 0 if file is empty and a positive number denoting the size
+ * of the file.
+ */
+extern ssize_t file_size(const char *);
>>
>>
> So what I understand, you want something like this:
>
> +ssize_t file_size_not_zero(const char *filename)
> +{
> + struct stat st;
> + if (stat(filename, &st) < 0)
> + return -1;
> + return !!st.st_size);
> +}
For the purpose of bisect_reset(), Yes. BTW a similar function exist
in builtin/am.c with the name is_empty_file(). But as Christian points
out file_size() could help to refactor other parts of code.
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] dir: introduce file_size() to check the size of file
On Wed, Jun 8, 2016 at 10:13 AM, Eric Sunshine wrote:
>
> I think this would be clearer if you instead added a function to
> bisect--helper.c which operates at a semantically higher level than
> what you have here (and drop this file_size() function). Specifically,
> add a function which tells you precisely what you want to know:
> whether the file exists and has non-zero size. For instance, in
> bisect--helper.c:
>
> static int file_empty_or_missing(const char *path)
> {
> struct stat st;
> return stat(...) < 0 || st.st_size == 0;
> }
>
> Or, if you have more cases where you want to know that it exists with
> non-zero size, then name it file_non_empty() and reverse the sense of
> the return value.
After a quick grep it seemed to me that there are a few places were we
stat a file just to get its size, so I suggested the file_size()
because it could help refactor other parts of the code.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] dir: introduce file_size() to check the size of file
On 06/08/2016 09:57 AM, Pranit Bauva wrote:
Hey Eric,
On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine wrote:
On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva wrote:
dir: introduce file_size() to check the size of file
At times we require to see if the file is empty and get the size of the
file. By using stat we can get the file size without actually having to
open the file to check for its contents.
The sole caller of this function in patch 4/4 does so only to check if
the file exists; it doesn't even care about the file's size, thus
neither this function nor this patch seem justified and probably ought
to be dropped unless some better and stronger justification can be
shown.
Umm, actually there is a subtle difference between file_exists() and
file_size(). file_exists() *only* checks whether the file exists or
not while file_size() can also be used to check whether the file is
empty or not also see the implementation of both of them which shows
the difference. In fact it doesn't care at all about the file size.
Now there are a lot of instances in shell scripts where there are
quite some differences with -f and -s and some places *do care* about
this subtle difference. For eg. in bisect_reset() we test whether the
file .git/BISECT_START has some contents in it. But I guess I can add
some more justification in the commit message. What do you think?
Signed-off-by: Pranit Bauva
---
diff --git a/dir.c b/dir.c
@@ -2036,6 +2036,14 @@ int file_exists(const char *f)
+ssize_t file_size(const char *filename)
+{
+ struct stat st;
+ if (stat(filename, &st) < 0)
+ return -1;
+ return xsize_t(st.st_size);
+}
+
diff --git a/dir.h b/dir.h
@@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list *el);
+/*
+ * Return the size of the file `filename`. It returns -1 if error
+ * occurred, 0 if file is empty and a positive number denoting the size
+ * of the file.
+ */
+extern ssize_t file_size(const char *);
So what I understand, you want something like this:
+ssize_t file_size_not_zero(const char *filename)
+{
+ struct stat st;
+ if (stat(filename, &st) < 0)
+ return -1;
+ return !!st.st_size);
+}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] dir: introduce file_size() to check the size of file
On Wed, Jun 8, 2016 at 3:57 AM, Pranit Bauva wrote:
> On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine wrote:
>> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva wrote:
>>> dir: introduce file_size() to check the size of file
>>>
>>> At times we require to see if the file is empty and get the size of the
>>> file. By using stat we can get the file size without actually having to
>>> open the file to check for its contents.
>>
>> The sole caller of this function in patch 4/4 does so only to check if
>> the file exists; it doesn't even care about the file's size, thus
>> neither this function nor this patch seem justified and probably ought
>> to be dropped unless some better and stronger justification can be
>> shown.
>
> Umm, actually there is a subtle difference between file_exists() and
> file_size(). file_exists() *only* checks whether the file exists or
> not while file_size() can also be used to check whether the file is
> empty or not also see the implementation of both of them which shows
> the difference. In fact it doesn't care at all about the file size.
> Now there are a lot of instances in shell scripts where there are
> quite some differences with -f and -s and some places *do care* about
> this subtle difference. For eg. in bisect_reset() we test whether the
> file .git/BISECT_START has some contents in it. But I guess I can add
> some more justification in the commit message. What do you think?
See my review of patch 4/4 which points out that C bisect_reset() does
*not* presently care about the file size, which is probably a bug
since that doesn't match the behavior of the shell code it's replacing
(which does care that the file is not empty).
I think this would be clearer if you instead added a function to
bisect--helper.c which operates at a semantically higher level than
what you have here (and drop this file_size() function). Specifically,
add a function which tells you precisely what you want to know:
whether the file exists and has non-zero size. For instance, in
bisect--helper.c:
static int file_empty_or_missing(const char *path)
{
struct stat st;
return stat(...) < 0 || st.st_size == 0;
}
Or, if you have more cases where you want to know that it exists with
non-zero size, then name it file_non_empty() and reverse the sense of
the return value.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] dir: introduce file_size() to check the size of file
Hey Eric,
On Wed, Jun 8, 2016 at 1:07 PM, Eric Sunshine wrote:
> On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva wrote:
>> dir: introduce file_size() to check the size of file
>>
>> At times we require to see if the file is empty and get the size of the
>> file. By using stat we can get the file size without actually having to
>> open the file to check for its contents.
>
> The sole caller of this function in patch 4/4 does so only to check if
> the file exists; it doesn't even care about the file's size, thus
> neither this function nor this patch seem justified and probably ought
> to be dropped unless some better and stronger justification can be
> shown.
Umm, actually there is a subtle difference between file_exists() and
file_size(). file_exists() *only* checks whether the file exists or
not while file_size() can also be used to check whether the file is
empty or not also see the implementation of both of them which shows
the difference. In fact it doesn't care at all about the file size.
Now there are a lot of instances in shell scripts where there are
quite some differences with -f and -s and some places *do care* about
this subtle difference. For eg. in bisect_reset() we test whether the
file .git/BISECT_START has some contents in it. But I guess I can add
some more justification in the commit message. What do you think?
>> Signed-off-by: Pranit Bauva
>> ---
>> diff --git a/dir.c b/dir.c
>> @@ -2036,6 +2036,14 @@ int file_exists(const char *f)
>> +ssize_t file_size(const char *filename)
>> +{
>> + struct stat st;
>> + if (stat(filename, &st) < 0)
>> + return -1;
>> + return xsize_t(st.st_size);
>> +}
>> +
>> diff --git a/dir.h b/dir.h
>> @@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list *el);
>> +/*
>> + * Return the size of the file `filename`. It returns -1 if error
>> + * occurred, 0 if file is empty and a positive number denoting the size
>> + * of the file.
>> + */
>> +extern ssize_t file_size(const char *);
Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] dir: introduce file_size() to check the size of file
On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva wrote:
> dir: introduce file_size() to check the size of file
>
> At times we require to see if the file is empty and get the size of the
> file. By using stat we can get the file size without actually having to
> open the file to check for its contents.
The sole caller of this function in patch 4/4 does so only to check if
the file exists; it doesn't even care about the file's size, thus
neither this function nor this patch seem justified and probably ought
to be dropped unless some better and stronger justification can be
shown.
> Signed-off-by: Pranit Bauva
> ---
> diff --git a/dir.c b/dir.c
> @@ -2036,6 +2036,14 @@ int file_exists(const char *f)
> +ssize_t file_size(const char *filename)
> +{
> + struct stat st;
> + if (stat(filename, &st) < 0)
> + return -1;
> + return xsize_t(st.st_size);
> +}
> +
> diff --git a/dir.h b/dir.h
> @@ -248,6 +248,13 @@ extern void clear_exclude_list(struct exclude_list *el);
> +/*
> + * Return the size of the file `filename`. It returns -1 if error
> + * occurred, 0 if file is empty and a positive number denoting the size
> + * of the file.
> + */
> +extern ssize_t file_size(const char *);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
