Re: [PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags
One more quesestion regarding flags used in structures : for example: update_callback_data has a flags field (type int) , in function void update_callback() the field flags of that structure is used as param for add_file_to_index(..., data->flags) and this function is define as: int add_file_to_index(..., int flags) in read-cache.c The question is : If the flags field of the structure is used in function calls should i update flags that deep?(there are other cases where the field is used in nested calls ) On Sun, Apr 2, 2017 at 6:30 AM, Junio C Hamano <gits...@pobox.com> wrote: > Robert Stanca <robert.stan...@gmail.com> writes: > >> I am used to 1change per patch so it's easier to redo specific >> patches...if there are small changes(10 lines max) can i send them as >> 1 patch? > > It's not number of lines. One line per patch does not make sense if > you need to make corresponding changes to two places, one line each, > in order to make the end result consistent. If you change a type of > a structure field, and that field is assigned to a variable > somewhere, you would change the type of both that field and the > variable that receives its value at the same time in a single > commit, as that would be the logical unit of a smallest change that > still makes sense (i.e. either half of that change alone would not > make sense). > > >
Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned
Regarding the first part , i can resend those 2 patches rewriting the
commit message if you want.
On Sat, Apr 1, 2017 at 10:12 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Robert Stanca <robert.stan...@gmail.com> writes:
>
>> Subject: Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned
>
> Try
>
> git shortlog --no-merges -40
>
> to learn how commits are typically titled. And then imagine this
> patch makes into our codebase and appear in the output.
>
> Can you tell what this commit is about from that single line title?
> No. You wouldn't even know that it is only touching bisect.h and
> nothing else.
>
> What do your readers want "shortlog" output to tell them about your
> commit? What are the most important thing (other than giving you an
> excuse to say "I have completed a microproject and now I am eligible
> to apply to GSoC" ;-)? Your proposed commit log message, especially
> its title, must be written to help future readers of the project
> history.
>
> Perhaps
>
> bisect.h: make flags field in rev_list_info unsigned
>
> would help them better.
>
>> Unsigned int is a closer representation of bitflags rather than signed int
>> that uses 1 special bit for sign.This shouldn't make much difference because
>> rev_list_info.flags uses only 2 bits(BISECT_SHOW_ALL and REV_LIST_QUIET)
>
> Overlong lines, without space after full-stop before the second
> sentence, without full-stop at the end of the sentence.
>
>>
>> Signed-off-by: Robert Stanca <robert.stan...@gmail.com>
>> ---
>> bisect.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/bisect.h b/bisect.h
>> index acd12ef80..a979a7f11 100644
>> --- a/bisect.h
>> +++ b/bisect.h
>> @@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct
>> commit_list *list,
>>
>> struct rev_list_info {
>> struct rev_info *revs;
>> - int flags;
>> + unsigned int flags;
>
> Have you checked how this field is used? For example, there is this
> line somewhere in rev-list.c
>
> int cnt, flags = info->flags;
>
> and the reason why the code copies the value to a local variable
> "flags" must be because it is going to use it, just like it and
> other codepaths use info->flags, no? It makes the code inconsistent
> by leaving the local variable a signed int, I suspect.
Re: [PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags
I am used to 1change per patch so it's easier to redo specific patches...if there are small changes(10 lines max) can i send them as 1 patch? On Sat, Apr 1, 2017 at 10:13 PM, Junio C Hamano <gits...@pobox.com> wrote: > Robert Stanca <robert.stan...@gmail.com> writes: > >> As rev_list_info's flag is unsigned int , var flags should have proper >> type.Also var cnt could be unsigned as there's no negative number of commits >> (all-reaches) >> >> Signed-off-by: Robert Stanca <robert.stan...@gmail.com> >> --- > > OK. I would have squashed these two commits into one, though.
[PATCH 1/2] [GSOC] Convert signed flags to unsigned
Unsigned int is a closer representation of bitflags rather than signed int
that uses 1 special bit for sign.This shouldn't make much difference because
rev_list_info.flags uses only 2 bits(BISECT_SHOW_ALL and REV_LIST_QUIET)
Signed-off-by: Robert Stanca <robert.stan...@gmail.com>
---
bisect.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bisect.h b/bisect.h
index acd12ef80..a979a7f11 100644
--- a/bisect.h
+++ b/bisect.h
@@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct commit_list
*list,
struct rev_list_info {
struct rev_info *revs;
- int flags;
+ unsigned int flags;
int show_timestamp;
int hdr_termination;
const char *header_prefix;
--
2.12.2.575.gb14f27f91.dirty
[PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags
As rev_list_info's flag is unsigned int , var flags should have proper
type.Also var cnt could be unsigned as there's no negative number of commits
(all-reaches)
Signed-off-by: Robert Stanca <robert.stan...@gmail.com>
---
builtin/rev-list.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0aa93d589..eb4af53ab 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -211,7 +211,7 @@ static void print_var_int(const char *var, int val)
static int show_bisect_vars(struct rev_list_info *info, int reaches, int all)
{
- int cnt, flags = info->flags;
+ unsigned int cnt, flags = info->flags;
char hex[GIT_SHA1_HEXSZ + 1] = "";
struct commit_list *tried;
struct rev_info *revs = info->revs;
--
2.12.2.575.gb14f27f91.dirty
Re: [PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator
On Sat, Apr 1, 2017 at 5:29 AM, Junio C Hamano <gits...@pobox.com> wrote: > > Robert Stanca <robert.stan...@gmail.com> writes: > > > Replaces recursive traversing of opendir with dir_iterator > > The original code for this one, and also the other one, is not > recursive traversing. This one enumerates all the _direct_ > subdirectories of ".git/worktrees", and feeds them to > prune_worktree() without recursing. The other one scans all the > files _directly_ underneath ".git/objects/pack" to find the ones > that begin with the packtmp prefix, and unlinks them without > recursing. Neither of them touches anything in subdirectory of > ".git/worktrees/" nor ".git/objects/pack/". > > Using dir_iterator() to make it recursive is not just overkill but > is a positively wrong change, isn't it? Thanks for the review, and yes the commit message is misleading (my bad there). I understood that remove_temp_files has no recursion component to it and it removes all ".tmp-id-pack-" files from /objects/pack , but shouldn't dir-iterator work even if there's no recursion level? My understanding of dir-iterator is that it is used for directory iteration (recursive or not) and where are no subdirectories it's basically recursive with level = 0 .
[PATCH] [GSOC] prune_worktrees(): reimplement with dir_iterator
Replaces recursive traversing of opendir with dir_iterator
Signed-off-by: Robert Stanca <robert.stan...@gmail.com>
---
builtin/worktree.c | 17 +++--
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9993ded..7cfd78c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -10,6 +10,8 @@
#include "refs.h"
#include "utf8.h"
#include "worktree.h"
+#include "iterator.h"
+#include "dir-iterator.h"
static const char * const worktree_usage[] = {
N_("git worktree add [] []"),
@@ -91,30 +93,25 @@ static void prune_worktrees(void)
{
struct strbuf reason = STRBUF_INIT;
struct strbuf path = STRBUF_INIT;
- DIR *dir = opendir(git_path("worktrees"));
- struct dirent *d;
+ struct dir_iterator *diter = dir_iterator_begin(git_path("worktrees"));
int ret;
- if (!dir)
- return;
- while ((d = readdir(dir)) != NULL) {
- if (is_dot_or_dotdot(d->d_name))
- continue;
+
+ while (dir_iterator_advance(diter) == ITER_OK) {
strbuf_reset();
- if (!prune_worktree(d->d_name, ))
+ if (!prune_worktree(diter->relative_path, ))
continue;
if (show_only || verbose)
printf("%s\n", reason.buf);
if (show_only)
continue;
strbuf_reset();
- strbuf_addstr(, git_path("worktrees/%s", d->d_name));
+ strbuf_addstr(, git_path("worktrees/%s",
diter->relative_path));
ret = remove_dir_recursively(, 0);
if (ret < 0 && errno == ENOTDIR)
ret = unlink(path.buf);
if (ret)
error_errno(_("failed to remove '%s'"), path.buf);
}
- closedir(dir);
if (!show_only)
rmdir(git_path("worktrees"));
strbuf_release();
--
2.7.4
[PATCH] [GSOC] remove_temporary_files(): reimplement using iterators
Replaces recursive traversing of opendir with dir_iterator
Signed-off-by: Robert Stanca <robert.stan...@gmail.com>
---
builtin/repack.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 677bc7c81..dba465814 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -7,6 +7,8 @@
#include "strbuf.h"
#include "string-list.h"
#include "argv-array.h"
+#include "iterator.h"
+#include "dir-iterator.h"
static int delta_base_offset = 1;
static int pack_kept_objects = -1;
@@ -49,12 +51,7 @@ static void remove_temporary_files(void)
{
struct strbuf buf = STRBUF_INIT;
size_t dirlen, prefixlen;
- DIR *dir;
- struct dirent *e;
-
- dir = opendir(packdir);
- if (!dir)
- return;
+ struct dir_iterator *diter = dir_iterator_begin(packdir);
/* Point at the slash at the end of ".../objects/pack/" */
dirlen = strlen(packdir) + 1;
@@ -62,14 +59,13 @@ static void remove_temporary_files(void)
/* Hold the length of ".tmp-%d-pack-" */
prefixlen = buf.len - dirlen;
- while ((e = readdir(dir))) {
- if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))
+ while (dir_iterator_advance(diter) == ITER_OK) {
+ if (strncmp(diter->relative_path, buf.buf + dirlen, prefixlen))
continue;
strbuf_setlen(, dirlen);
- strbuf_addstr(, e->d_name);
+ strbuf_addstr(, diter->relative_path);
unlink(buf.buf);
}
- closedir(dir);
strbuf_release();
}
--
2.12.2.575.gb14f27f91.dirty
[PATCH] [GSOC] get_non_kept_pack_filenames(): reimplement using iterators
Replaces recursive traversing of opendir with dir_iterator.
Signed-off-by: Robert Stanca <robert.stan...@gmail.com>
---
builtin/repack.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 677bc7c..27a5597 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -7,6 +7,8 @@
#include "strbuf.h"
#include "string-list.h"
#include "argv-array.h"
+#include "iterator.h"
+#include "dir-iterator.h"
static int delta_base_offset = 1;
static int pack_kept_objects = -1;
@@ -86,26 +88,21 @@ static void remove_pack_on_signal(int signo)
*/
static void get_non_kept_pack_filenames(struct string_list *fname_list)
{
- DIR *dir;
- struct dirent *e;
+ struct dir_iterator *diter = dir_iterator_begin(packdir);
char *fname;
- if (!(dir = opendir(packdir)))
- return;
-
- while ((e = readdir(dir)) != NULL) {
+ while (dir_iterator_advance(diter) == ITER_OK) {
size_t len;
- if (!strip_suffix(e->d_name, ".pack", ))
+ if (!strip_suffix(diter->relative_path, ".pack", ))
continue;
- fname = xmemdupz(e->d_name, len);
+ fname = xmemdupz(diter->relative_path, len);
if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
string_list_append_nodup(fname_list, fname);
else
free(fname);
}
- closedir(dir);
}
static void remove_redundant_pack(const char *dir_name, const char *base_name)
--
2.7.4
Hi , this is my first patch submission for Git Gsoc. I ran full tests and local
tests with
prove --timer --jobs 15 ./t*pack*.sh .
Have a great day,
Robert.
