| Message ID | 20230302220251.1474923-2-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | submodule: parallelize diff | expand |
Calvin Wan <calvinwan@google.com> writes: > A prepatory change for a future patch that moves the status parsing > logic to a separate function. > > Signed-off-by: Calvin Wan <calvinwan@google.com> > --- > submodule.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > Subject: Re: [PATCH v9 2/6] submodule: rename strbuf variable What strbuf variable renamed to what? I have a feeling that squashing this and 3/6 into a single patch, and pass buf.buf and buf.len to the new helper function without introducing an intermediate variables in the caller, would make the resulting code easier to follow. In any case, nice factoring out of a useful helper function. > diff --git a/submodule.c b/submodule.c > index fae24ef34a..faf37c1101 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1906,25 +1906,28 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked) > > fp = xfdopen(cp.out, "r"); > while (strbuf_getwholeline(&buf, fp, '\n') != EOF) { > + char *str = buf.buf; > + const size_t len = buf.len; > + > /* regular untracked files */ > - if (buf.buf[0] == '?') > + if (str[0] == '?') > dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED; > > - if (buf.buf[0] == 'u' || > - buf.buf[0] == '1' || > - buf.buf[0] == '2') { > + if (str[0] == 'u' || > + str[0] == '1' || > + str[0] == '2') { > /* T = line type, XY = status, SSSS = submodule state */ > - if (buf.len < strlen("T XY SSSS")) > + if (len < strlen("T XY SSSS")) > BUG("invalid status --porcelain=2 line %s", > - buf.buf); > + str); > > - if (buf.buf[5] == 'S' && buf.buf[8] == 'U') > + if (str[5] == 'S' && str[8] == 'U') > /* nested untracked file */ > dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED; > > - if (buf.buf[0] == 'u' || > - buf.buf[0] == '2' || > - memcmp(buf.buf + 5, "S..U", 4)) > + if (str[0] == 'u' || > + str[0] == '2' || > + memcmp(str + 5, "S..U", 4)) > /* other change */ > dirty_submodule |= DIRTY_SUBMODULE_MODIFIED; > }
On Thu, Mar 2, 2023 at 4:25 PM Junio C Hamano <gitster@pobox.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > A prepatory change for a future patch that moves the status parsing > > logic to a separate function. > > > > Signed-off-by: Calvin Wan <calvinwan@google.com> > > --- > > submodule.c | 23 +++++++++++++---------- > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > Subject: Re: [PATCH v9 2/6] submodule: rename strbuf variable > > What strbuf variable renamed to what? > > I have a feeling that squashing this and 3/6 into a single patch, > and pass buf.buf and buf.len to the new helper function without > introducing an intermediate variables in the caller, would make the > resulting code easier to follow. > > In any case, nice factoring out of a useful helper function. > A much earlier version squashed those changes together, but it was recommended to split those changes up; I think I am indifferent either way since the refactoring is clear to me whether it is split up or not. https://lore.kernel.org/git/221012.868rllo545.gmgdl@evledraar.gmail.com/
Calvin Wan <calvinwan@google.com> writes: > On Thu, Mar 2, 2023 at 4:25 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Calvin Wan <calvinwan@google.com> writes: >> >> > A prepatory change for a future patch that moves the status parsing >> > logic to a separate function. >> > >> > Signed-off-by: Calvin Wan <calvinwan@google.com> >> > --- >> > submodule.c | 23 +++++++++++++---------- >> > 1 file changed, 13 insertions(+), 10 deletions(-) >> >> > Subject: Re: [PATCH v9 2/6] submodule: rename strbuf variable >> >> What strbuf variable renamed to what? >> >> I have a feeling that squashing this and 3/6 into a single patch, >> and pass buf.buf and buf.len to the new helper function without >> introducing an intermediate variables in the caller, would make the >> resulting code easier to follow. >> >> In any case, nice factoring out of a useful helper function. >> > > A much earlier version squashed those changes together, but it was > recommended to split those changes up; I think I am indifferent either way > since the refactoring is clear to me whether it is split up or not. > https://lore.kernel.org/git/221012.868rllo545.gmgdl@evledraar.gmail.com/ I am indifferent, either, but with or without them squashed into a single patch, "rename strbuf" would not be how you would describe the value of this refactoring, which is to make the interface not depend on strbuf. Some callers may have separate <ptr,len> pair that is not in strbuf, and with the current interface they are forced to wrap the pair in a throw-away strbuf which is not nice. And squashing them together into a single patch, it becomes a lot clear what the point of these two steps combined is.
On Mon, Mar 6, 2023 at 10:30 AM Junio C Hamano <gitster@pobox.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > On Thu, Mar 2, 2023 at 4:25 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Calvin Wan <calvinwan@google.com> writes: > >> > >> > A prepatory change for a future patch that moves the status parsing > >> > logic to a separate function. > >> > > >> > Signed-off-by: Calvin Wan <calvinwan@google.com> > >> > --- > >> > submodule.c | 23 +++++++++++++---------- > >> > 1 file changed, 13 insertions(+), 10 deletions(-) > >> > >> > Subject: Re: [PATCH v9 2/6] submodule: rename strbuf variable > >> > >> What strbuf variable renamed to what? > >> > >> I have a feeling that squashing this and 3/6 into a single patch, > >> and pass buf.buf and buf.len to the new helper function without > >> introducing an intermediate variables in the caller, would make the > >> resulting code easier to follow. > >> > >> In any case, nice factoring out of a useful helper function. > >> > > > > A much earlier version squashed those changes together, but it was > > recommended to split those changes up; I think I am indifferent either way > > since the refactoring is clear to me whether it is split up or not. > > https://lore.kernel.org/git/221012.868rllo545.gmgdl@evledraar.gmail.com/ > > I am indifferent, either, but with or without them squashed into a > single patch, "rename strbuf" would not be how you would describe > the value of this refactoring, which is to make the interface not > depend on strbuf. Some callers may have separate <ptr,len> pair > that is not in strbuf, and with the current interface they are > forced to wrap the pair in a throw-away strbuf which is not nice. I see what you mean here; will reword the commit message, thanks!
diff --git a/submodule.c b/submodule.c index fae24ef34a..faf37c1101 100644 --- a/submodule.c +++ b/submodule.c @@ -1906,25 +1906,28 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked) fp = xfdopen(cp.out, "r"); while (strbuf_getwholeline(&buf, fp, '\n') != EOF) { + char *str = buf.buf; + const size_t len = buf.len; + /* regular untracked files */ - if (buf.buf[0] == '?') + if (str[0] == '?') dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED; - if (buf.buf[0] == 'u' || - buf.buf[0] == '1' || - buf.buf[0] == '2') { + if (str[0] == 'u' || + str[0] == '1' || + str[0] == '2') { /* T = line type, XY = status, SSSS = submodule state */ - if (buf.len < strlen("T XY SSSS")) + if (len < strlen("T XY SSSS")) BUG("invalid status --porcelain=2 line %s", - buf.buf); + str); - if (buf.buf[5] == 'S' && buf.buf[8] == 'U') + if (str[5] == 'S' && str[8] == 'U') /* nested untracked file */ dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED; - if (buf.buf[0] == 'u' || - buf.buf[0] == '2' || - memcmp(buf.buf + 5, "S..U", 4)) + if (str[0] == 'u' || + str[0] == '2' || + memcmp(str + 5, "S..U", 4)) /* other change */ dirty_submodule |= DIRTY_SUBMODULE_MODIFIED; }
A prepatory change for a future patch that moves the status parsing logic to a separate function. Signed-off-by: Calvin Wan <calvinwan@google.com> --- submodule.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)