On 2022-04-13 at 20:10:07, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > On 2022-04-12 at 20:14:34, Jonathan Tan wrote:
> >> This seems like you're using the commit message as the reflog message -
> >> is this necessary? For what it's worth, all tests still pass if I
> >> replace "msg" with "NULL".
> >
> > I think that's what the existing stash code does, and so I did the same
> > here.  It's not strictly necessary, but it's a nice to have.
> >
> > I didn't think it worth testing, because I don't think we test it
> > elsewhere, either.
> >
> >> It might be worth adding tests that check that the exported stashes are
> >> in the expected format (to ensure that we can read stashes exported from
> >> another Git version) but I don't think that has to block the submission
> >> of this patch set.
> >
> > There's a tiny patch for that for the base commit, but you're right that
> > some more tests wouldn't hurt.  I can send a followup patch or two as
> > part of a new series.
> 
> Is this about the log messages recorded in the throw-away commits
> that are only used to form a single backbone chain, to which the
> commits used to represent stash entries are linked to?

My response to the first paragraph was talking about the messages in the
reflog.  When we create a reflog entry, we add a message, which I've set
to the commit message of the stash entry, like the existing code does.
I don't think that's an important detail, but I did it to be consistent.
I think it's better to put something useful there, at least, rather than
not put any message at all.

The log messages recorded in the chain of throwaway commits are
identical to the corresponding stash commit's message with a prefix of
"git stash: ".  There's currently a test for the base commit having a
certain fixed value, but not the contents of the other commits.

> Are these messages meant to be used in any way?  I do not think
> these messages contribute anything to the end result (they are just
> discarded once they serve their purpose of transferring the
> underlying stash entries, if I recall the design discussion
> correctly), so I am not sure if we would even want to cast in stone
> what they would say.
> 
> If on the other hand they are meant to be read by something (either
> programs or end-user humans), it does make sense to ensure that we
> are recording what we think we are recording.

The log (not reflog) messages are valuable because since these message
are pushed as refs, someone may look at them (e.g., on GitHub or with
git log) and it is helpful to provide something that tells the user
what's going on.

For example, since our throwaway commits are empty, it would seem
bizarre to users that someone pushed a commit with an empty tree, but if
they see that the commits are stash commits, that may help them have
useful context.

I'm happy to add a few more tests for this in a followup series.  I'll
likely get to it this weekend and can include some checks for the commit
message and some improved verification of commits in the testsuite.  I
don't think this should hold up the rest of the series, however.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA