| Message ID | 5bfe3f3fc8a99b3d4fdd4286da17cd935090c614.1588857462.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | In-tree sparse-checkout definitions | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <dstolee@microsoft.com> > > The loop in warn_conflicted_path() that checks for the count of entries > with the same path uses "i+count" for the array entry. However, the loop > only verifies that the value of count is below the array size. Fix this > by adding i to the condition. > > I hit this condition during a test of the in-tree sparse-checkout > feature, so it is exercised by the end of the series. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > unpack-trees.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/unpack-trees.c b/unpack-trees.c > index 9a3ccd9d083..4f880f2da90 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate, > add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path); > > /* Find out how many higher stage entries at same path */ > - while (++count < istate->cache_nr && > + while (i + ++count < istate->cache_nr && > !strcmp(conflicting_path, > istate->cache[i+count]->name)) > /* do nothing */; Eek. Yes, it is obvious that the original is wrong once you point it out. But "i + ++count" looks like a line noise, and funny way that lines are wrapped in the original does not help X-<. We may want to fix the style and the grammar while we are at it, perhaps like the attached. In any case, thanks for a fix. unpack-trees.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index 6bbf58d28e..c38938d96c 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -562,11 +562,11 @@ static int warn_conflicted_path(struct index_state *istate, add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path); - /* Find out how many higher stage entries at same path */ - while (++count < istate->cache_nr && - !strcmp(conflicting_path, - istate->cache[i+count]->name)) - /* do nothing */; + /* Find out how many higher stage entries are at same path */ + while ((++count) + i < istate->cache_nr && + !strcmp(conflicting_path, istate->cache[count + i]->name)) + ; /* do nothing */ + return count; }
On 5/7/2020 6:27 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: >> diff --git a/unpack-trees.c b/unpack-trees.c >> index 9a3ccd9d083..4f880f2da90 100644 >> --- a/unpack-trees.c >> +++ b/unpack-trees.c >> @@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate, >> add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path); >> >> /* Find out how many higher stage entries at same path */ >> - while (++count < istate->cache_nr && >> + while (i + ++count < istate->cache_nr && >> !strcmp(conflicting_path, >> istate->cache[i+count]->name)) >> /* do nothing */; > > Eek. Yes, it is obvious that the original is wrong once you point > it out. But "i + ++count" looks like a line noise, and funny way > that lines are wrapped in the original does not help X-<. > > We may want to fix the style and the grammar while we are at it, > perhaps like the attached. > > In any case, thanks for a fix. > > unpack-trees.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/unpack-trees.c b/unpack-trees.c > index 6bbf58d28e..c38938d96c 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -562,11 +562,11 @@ static int warn_conflicted_path(struct index_state *istate, > > add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path); > > - /* Find out how many higher stage entries at same path */ > - while (++count < istate->cache_nr && > - !strcmp(conflicting_path, > - istate->cache[i+count]->name)) > - /* do nothing */; > + /* Find out how many higher stage entries are at same path */ > + while ((++count) + i < istate->cache_nr && > + !strcmp(conflicting_path, istate->cache[count + i]->name)) > + ; /* do nothing */ > + This looks much better, thanks! As I mentioned in the cover letter, this is worth taking on its own. Could you queue the collaborative patch? I'll eject it from the next version of this series. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > As I mentioned in the cover letter, this is worth taking on its own. Could > you queue the collaborative patch? I'll eject it from the next version of > this series. Yes, I think this is worth taking independently. I do not think people would find the updated version "much better", but it made it readable at least to me. Thanks.
On Thu, May 7, 2020 at 3:29 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Derrick Stolee <dstolee@microsoft.com> > > > > The loop in warn_conflicted_path() that checks for the count of entries > > with the same path uses "i+count" for the array entry. However, the loop > > only verifies that the value of count is below the array size. Fix this > > by adding i to the condition. > > > > I hit this condition during a test of the in-tree sparse-checkout > > feature, so it is exercised by the end of the series. > > > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > > --- > > unpack-trees.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/unpack-trees.c b/unpack-trees.c > > index 9a3ccd9d083..4f880f2da90 100644 > > --- a/unpack-trees.c > > +++ b/unpack-trees.c > > @@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate, > > add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path); > > > > /* Find out how many higher stage entries at same path */ > > - while (++count < istate->cache_nr && > > + while (i + ++count < istate->cache_nr && > > !strcmp(conflicting_path, > > istate->cache[i+count]->name)) > > /* do nothing */; > > Eek. Yes, it is obvious that the original is wrong once you point > it out. But "i + ++count" looks like a line noise, and funny way > that lines are wrapped in the original does not help X-<. Eek, indeed. :-( > We may want to fix the style and the grammar while we are at it, > perhaps like the attached. > > In any case, thanks for a fix. > > unpack-trees.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/unpack-trees.c b/unpack-trees.c > index 6bbf58d28e..c38938d96c 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -562,11 +562,11 @@ static int warn_conflicted_path(struct index_state *istate, > > add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path); > > - /* Find out how many higher stage entries at same path */ > - while (++count < istate->cache_nr && > - !strcmp(conflicting_path, > - istate->cache[i+count]->name)) > - /* do nothing */; > + /* Find out how many higher stage entries are at same path */ > + while ((++count) + i < istate->cache_nr && > + !strcmp(conflicting_path, istate->cache[count + i]->name)) > + ; /* do nothing */ > + > return count; > } Thanks, both.
diff --git a/unpack-trees.c b/unpack-trees.c index 9a3ccd9d083..4f880f2da90 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -563,10 +563,11 @@ static int warn_conflicted_path(struct index_state *istate, add_rejected_path(o, WARNING_SPARSE_UNMERGED_FILE, conflicting_path); /* Find out how many higher stage entries at same path */ - while (++count < istate->cache_nr && + while (i + ++count < istate->cache_nr && !strcmp(conflicting_path, istate->cache[i+count]->name)) /* do nothing */; + return count; }