gitproto, syncer: address PR #64 review feedback

2dc121d→main·

Soph·1mo ago·2 files·+25 added/-8

Three small fixes after Bugbot review:

• gitproto/push.go countingWriter.Write: wrap the underlying write error so wrapcheck stops failing the lint job (CI run 26288151165).
• gitproto/push.go PushObjects: pass the selection error into the stopSelect callback. Previously "selected N objects in X" was printed even when ObjectsToPack returned an error, which was misleading (count was 0 or partial, and the next line was the actual error). startSelectionProgress now suppresses the success summary when err != nil; the ticker still stops cleanly either way.
• syncer/progress.go sessionStderr: serialize Write under a mutex. The materialized-push refactor in this PR runs an encode-progress ticker concurrently with sendReceivePack, and both write to the same conn.ProgressWriter() (which is *sessionStderr). The strings.Builder buffer and the progress notify/setTransient calls weren't synchronized, so overlapping writes could scramble the live progress UI or, in theory, corrupt the buffer.

Sessions

7422ae335ce1View transcript

Changes

2

• internal

• gitproto

• Mpush.go+16/-8

• syncer

• Mprogress.go+9

231 unmodified lines

232
233
234
235
235
236
237
238
47 unmodified lines

286
287
288
289
289
290
291
292
293
294
295
1 unmodified line

297
298
299
297
298
299
300
301
302
303
304
305
306
307
308
304
309
310
306
311
312
313
314
11 unmodified lines

326
327
328
324
329
330
331
332
333
334
335
336
337
338

231 unmodified lines

stopSelect := startSelectionProgress(progressDest)
    objects, err := packfile.NewDeltaSelector(store).ObjectsToPack(hashes, 10)
    stopSelect(len(objects))
    stopSelect(len(objects), err)
    if err != nil {
        return fmt.Errorf("select objects to pack: %w", err)
    }
47 unmodified lines

func (cw *countingWriter) Write(p []byte) (int, error) {
    n, err := cw.w.Write(p)
    cw.n.Add(int64(n))
    return n, err
    if err != nil {
        return n, fmt.Errorf("counting writer: %w", err)
    }
    return n, nil
}

func (cw *countingWriter) Count() int64 { return cw.n.Load() }
1 unmodified line

// startSelectionProgress emits in-place "selecting deltas, elapsed X"
// updates every 500ms during the synchronous delta-selection phase of
// PushObjects. The returned stop function takes the number of selected
// objects and finalizes the line with a permanent "selected N objects
// in Y" summary. When dest is nil (non-verbose mode) returns a no-op
// stop, so callers don't need to special-case verbosity.
// objects and the selection error (nil on success); on success it
// finalizes the line with a permanent "selected N objects in Y"
// summary, on error it just stops the ticker without claiming success.
// When dest is nil (non-verbose mode) returns a no-op stop, so
// callers don't need to special-case verbosity.

// Selection has no observable byte progress — go-git's DeltaSelector
// is opaque to the caller — so elapsed time is the only signal we can
// surface to keep long selections from looking like a hang.
func startSelectionProgress(dest io.Writer) func(objectCount int) {
func startSelectionProgress(dest io.Writer) func(objectCount int, err error) {
    if dest == nil {
        return func(int) {}
        return func(int, error) {}
    }
    start := time.Now()
    ticker := time.NewTicker(500 * time.Millisecond)
11 unmodified lines

}
}
return func(objectCount int) {
return func(objectCount int, err error) {
    ticker.Stop()
    close(stop)
    <-done
    if err != nil {
        return
    }
    fmt.Fprintf(dest, "selected %d objects in %s\n",
        objectCount, time.Since(start).Round(time.Second))
    }

Minternal/gitproto/push.go+16/-8

348 unmodified lines

349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
5 unmodified lines

369
370
371
372
373
374
375
376

348 unmodified lines

// line in two calls — first the prefix ("source: \"), then the content
// with terminator — and would otherwise produce two separate notify
// frames split mid-line. Use as a pointer (the buffer is stateful).
//
// mu guards buf and serializes notify/setTransient calls against
// concurrent writers. The HTTP push path is the case that motivated
// this: a materialized-push encode goroutine and the receive-pack
// response demuxer can both emit progress through the same
// conn.ProgressWriter() during overlapping windows.
type sessionStderr struct {
    s   *syncSession
    mu  sync.Mutex
    buf strings.Builder
}

5 unmodified lines

}
    return n, nil
    }
    w.mu.Lock()
    defer w.mu.Unlock()
    s := string(b)
    for s != "" {
        i := strings.IndexAny(s, "\r\n")